Skip to content

fix: SwitchToAsync compat with local SDK build#647

Closed
PureWeen wants to merge 2 commits intomainfrom
fix/local-sdk-switchto-compat
Closed

fix: SwitchToAsync compat with local SDK build#647
PureWeen wants to merge 2 commits intomainfrom
fix/local-sdk-switchto-compat

Conversation

@PureWeen
Copy link
Copy Markdown
Owner

One-line fix: removes the extra null arg from SwitchToAsync so it compiles against the local SDK (3-arg signature) instead of only v0.2.2 (4-arg with ModelCapabilitiesOverride).

The local SDK build (0.3.0-local) has a 3-arg SwitchToAsync signature
(modelId, reasoningEffort, cancellationToken) while v0.2.2 added a
4th ModelCapabilitiesOverride parameter. Remove the null placeholder
to compile against both.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PureWeen
Copy link
Copy Markdown
Owner Author

🔍 Multi-Model Code Review — PR #647

Reviewed by: Direct review (1-line diff, no sub-agent dispatch needed)
Diff reviewed: +1/−1, 1 file
CI Status: ⚠️ Expert Code Review pending


Findings

🟡 MODERATE — #1: Breaks published SDK v0.2.2 build

File: CopilotService.cs, line 3316

The PR removes the null argument for ModelCapabilitiesOverride? from SwitchToAsync:

// Before (4-arg, matches SDK v0.2.2):
await state.Session.Rpc.Model.SwitchToAsync(normalizedModel, reasoningEffort, null, cancellationToken);

// After (3-arg, matches local SDK build):
await state.Session.Rpc.Model.SwitchToAsync(normalizedModel, reasoningEffort, cancellationToken);

SDK v0.2.2 on NuGet only has the 4-arg overload — the 3-arg was removed in the v0.2.1→v0.2.2 upgrade. This PR compiles against a local SDK build (likely 0.3.0-local from PureWeen/copilot-sdk) that still has the old 3-arg signature.

Impact: Anyone building from the published NuGet package (v0.2.2) will get a compile error: CS1501: No overload for method 'SwitchToAsync' takes 3 arguments.

Recommendation: This is fine if the intent is to track the local SDK build temporarily. If so, consider adding a comment noting the SDK version dependency. If this should work with both, use #if conditional compilation or keep the 4-arg call.


Recommended Action

✅ Approve (conditional)

The change is correct for the local SDK build. The risk is limited since the use-local-sdk.sh script and local nuget source would need to be configured for this to compile anyway. Approve with the understanding this is a temporary local-SDK-tracking change.

@PureWeen
Copy link
Copy Markdown
Owner Author

Good catch — this is intentionally tracking the local SDK build which reverted to the 3-arg signature. The Directory.Build.props.local override is active, so the published v0.2.2 NuGet package is not used. When we move back to a published SDK version, this call will need to be updated to match that version's signature.

- McpLocalServerConfig → McpStdioServerConfig
- McpRemoteServerConfig → McpHttpServerConfig
- Dictionary<string, object> → Dictionary<string, McpServerConfig>
- ParseMcpServerConfig returns McpServerConfig (not object)
- Remove config.Type setter (now read-only virtual override)
- SystemNotificationDataKindXxx → SystemNotificationXxx
- UserMessageDataAttachmentsItem → UserMessageAttachment
- Restore 4-arg SwitchToAsync (v0.3 re-added ModelCapabilitiesOverride)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PureWeen
Copy link
Copy Markdown
Owner Author

Closing — the v0.3 SDK type renames (McpStdioServerConfig, SystemNotificationXxx, etc.) break compatibility with the published v0.2.2 NuGet package. The code on main already builds clean against v0.2.2. We'll update when v0.3 is published.

@PureWeen PureWeen closed this Apr 20, 2026
@PureWeen PureWeen deleted the fix/local-sdk-switchto-compat branch April 20, 2026 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant