Carry over the remaining SessionConfig fields in copilot copyResumeSessionConfig so multi-turn options persist - #710
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a create-vs-resume configuration asymmetry in copilotprovider.openSession by ensuring multi-turn sessions retain the same effective copilot.SessionConfig options after resuming with an existing service ID.
Changes:
- Expanded
copyResumeSessionConfigto carry over additionalSessionConfigfields intocopilot.ResumeSessionConfig, restoring multi-turn option persistence. - Extended
richSessionConfig()andTestCopyResumeSessionConfig_CopiesAllPropertiesto cover the newly preserved fields on thesession.resumewire request.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| provider/copilotprovider/copilot.go | Copies additional session config fields into resume config so resume path matches create path behavior. |
| provider/copilotprovider/copilot_test.go | Adds richer config fixtures and assertions to verify new fields are sent on session.resume. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ReasoningEffort: source.ReasoningEffort, | ||
| ReasoningSummary: source.ReasoningSummary, | ||
| ContextTier: source.ContextTier, | ||
| Tools: source.Tools, |
There was a problem hiding this comment.
Good catch — fixed in f989f62: copyResumeSessionConfig now sets Tools: slices.Clone(source.Tools) so the per-turn append in resumeSessionConfig can no longer mutate the shared base SessionConfig backing array.
This comment has been minimized.
This comment has been minimized.
14006db to
11b0d4d
Compare
This comment has been minimized.
This comment has been minimized.
The first turn of a copilot-backed agent has no service ID and creates the session via copySessionConfig, which clones the full SessionConfig. Later turns carry a service ID and go through copyResumeSessionConfig, which hand-copied only 18 of the fields shared with copilot.ResumeSessionConfig. Every other configured option (ClientName, Providers/Models, Capi, ReasoningSummary, ContextTier, the Enable*/Skip* toggles, DefaultAgent/Agent, PluginDirectories, GitHubToken, RemoteSession, and more) was silently dropped, so a multi-turn agent's configuration quietly stopped taking effect after the first turn. Mirror the create path by copying every SessionConfig field that also exists on ResumeSessionConfig, matching the SDK's own CreateSession/ResumeSession parity. Extend the rich fixture and the resume test to assert the wire params now match the create request.
…ting shared config
f989f62 to
9f49289
Compare
Parity Review — PR #710Scope: Verdict: ✅ No parity issues. No exported API changes. This PR is a bug fix entirely within unexported implementation helpers ( Cross-repo consistency: The concept of mapping shared session configuration fields uniformly between a create-session path and a resume-session path is a Go-internal implementation detail of the Copilot provider wrapper. There is no analogous Label: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
What
copilotprovider.openSessionuses two paths to build the SDK session config:copySessionConfig, which doesclone := *sourceand therefore preserves everycopilot.SessionConfigfield.resumeSessionConfig->copyResumeSessionConfig, which hand-copied only 18 fields.The SDK's
copilot.ResumeSessionConfigshares far more fields withSessionConfigthan those 18. The rest —ClientName,GitHubToken,Providers/Models/Capi,ReasoningSummary,ContextTier,ModelCapabilities,OnMCPAuthRequest,MCPOAuthTokenStorage,DefaultAgent/Agent/ExcludedBuiltInAgents,PluginDirectories,InstructionDirectories,LargeOutput,ToolSearch,Memory,RemoteSession,EnableSessionTelemetry,EnableCitations,SessionLimits,EnableConfigDiscovery,SkipEmbeddingRetrieval,EmbeddingCacheStorage,OrganizationCustomInstructions,EnableOnDemandInstructionDiscovery,EnableFileHooks,EnableHostGitOperations,EnableSessionStore,EnableSkills,IncludeSubAgentStreamingEvents,SkipCustomInstructions,CustomAgentsLocalOnly,CoauthorEnabled,ManageScheduleEnabled— were silently dropped.The result: a multi-turn agent's configured options quietly stopped taking effect after the first turn.
Fix
Extend
copyResumeSessionConfigto carry over everySessionConfigfield that also exists oncopilot.ResumeSessionConfig, mirroring the create path. Each added field was verified to exist on both structs in the vendored SDK. Streaming still defaults viacopyBoolDefaultTrue.Why (parity)
This mirrors the copilot SDK's own
Client.CreateSession/Client.ResumeSessionWithOptions, which map the same shared set of options onto the create and resume wire requests. The create-vs-resume asymmetry in the provider was the sole reason those options were honored on turn 1 but not afterward; carrying them over restores the intended per-session configuration across turns.Testing
richSessionConfig()to populate the previously-dropped fields (ClientName,Providers/Models,ContextTier,ReasoningSummary,DefaultAgent/Agent,EnableSkills,GitHubToken,RemoteSession, theEnable*/Skip*toggles, etc.).TestCopyResumeSessionConfig_CopiesAllPropertiesto resume an existing-service-ID session and assert the corresponding wire params are now present on thesession.resumerequest and match what the create path sends.clientName = <nil>, want "test-client") and passes after.go build ./...,go vet ./provider/copilotprovider/..., andgo test ./provider/copilotprovider/...all pass.