Add CopilotSdk product: IChatClient adapter for GitHub Copilot SDK#278
Add CopilotSdk product: IChatClient adapter for GitHub Copilot SDK#278mattleibow wants to merge 9 commits into
Conversation
- Microsoft.Maui.CopilotSdk: IChatClient adapter wrapping CopilotClient/CopilotSession with streaming, tool calling, session management, and DI support - CopilotSdkSample: Minimal MAUI chat app demonstrating streaming responses and tools - Unit + integration tests in tests/CopilotSdk/ - CI workflow, CPM entries, signing config, solution filter Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add missing xmlns/xmlns:x declarations to App.xaml, AppShell.xaml, Colors.xaml, Styles.xaml - Add DevFlow agent to sample app for UI inspection/testing - Disable Mac Catalyst sandbox so DevFlow agent can bind TCP port - Verified: app launches, DevFlow responds on port 9224, chat with tool calling works end-to-end (get_current_time tool invoked) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
IChatClient adapter improvements: - Map ChatOptions.ModelId to session model (resets session on change) - Map ChatOptions.ResponseFormat (JSON mode via system message) - Surface AssistantReasoningDeltaEvent for reasoning models - Map ReasoningEffort via ChatOptions.AdditionalProperties - Support image/vision inputs via DataContent → SDK attachments - Expose ListModelsAsync() for model discovery - Include ModelId in ChatResponse/ChatResponseUpdate Repo structure: - Move sample from src/CopilotSdk/ to samples/ (repo convention) - Update MauiLabs.slnx, CopilotSdk.slnf, CI workflow paths Tests: - DI registration tests (AddCopilotSdkChatClient, config action) - GetService type tests - Custom StreamingTimeout test - Empty tools list / null options edge cases Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reasoning-tagged TextContent from AssistantReasoningDeltaEvent is now excluded from the primary response text in GetResponseAsync. Tool call/result content types are preserved in the response. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pass all AIContent items through to ChatMessage instead of joining into a single string. Filter out reasoning-tagged TextContent so chain-of-thought doesn't appear in the non-streaming response. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Model picker (gpt-4.1, gpt-4o, claude-sonnet-4.5, o4-mini) - JSON mode toggle switch - Session reset button - Reasoning chunks shown in purple italic bubbles - Tool calls shown as 🔧 yellow bubbles - Tool results shown as ✅ green bubbles - Error messages shown as⚠️ red bubbles - System messages shown as blue centered bubbles - Distinct MessageKind enum for all bubble types Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new Microsoft.Maui.CopilotSdk product that adapts the GitHub.Copilot.SDK to the standard Microsoft.Extensions.AI.IChatClient interface, plus a MAUI sample app, tests, and the build/CI infrastructure to ship it as a NuGet package.
Changes:
- New
CopilotSdkChatClient(IChatClientadapter) with streaming, tool calling, sessions, vision, reasoning, and DI extension method. - New
CopilotSdkSampleMAUI app (chat UI with model picker, JSON toggle, session reset, DevFlow agent on port 9224) and a unit + integration test project. - Build wiring: package added to CPM (
Directory.Packages.props,eng/Versions.props), signing entry, solution filter,MauiLabs.slnxentries, GitHub Actions CI workflow.
Reviewed changes
Copilot reviewed 44 out of 50 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/CopilotSdk/Microsoft.Maui.CopilotSdk/CopilotSdkChatClient.cs | Core adapter mapping SDK events to IAsyncEnumerable<ChatResponseUpdate> |
| src/CopilotSdk/Microsoft.Maui.CopilotSdk/CopilotSdkConfiguration.cs | Configuration POCO (model, system message, auth, CLI path) |
| src/CopilotSdk/Microsoft.Maui.CopilotSdk/CopilotSdkServiceCollectionExtensions.cs | AddCopilotSdkChatClient DI extension |
| src/CopilotSdk/Microsoft.Maui.CopilotSdk/Microsoft.Maui.CopilotSdk.csproj | Shipping package csproj |
| src/CopilotSdk/Microsoft.Maui.CopilotSdk/README.md, src/CopilotSdk/README.md | NuGet + product READMEs |
| src/CopilotSdk/CopilotSdk.slnf | Solution filter for the new product |
| tests/CopilotSdk/Microsoft.Maui.CopilotSdk.Tests/* | Unit + integration tests for adapter, DI, lifecycle |
| samples/CopilotSdkSample/* | MAUI sample chat app (XAML, view model, MauiProgram, platform heads, resources) |
| MauiLabs.slnx | Adds new product, sample, and test projects to the root solution |
| eng/Versions.props, Directory.Packages.props | Adds GitHub.Copilot.SDK 0.3.0 to CPM |
| eng/Signing.props | Adds 3rd-party signing entry for GitHub.Copilot.SDK.dll |
| .github/workflows/ci-copilotsdk.yml | PR/push CI workflow for the new product |
| public void Dispose() | ||
| { | ||
| if (_disposed) return; | ||
| _disposed = true; | ||
| _ = DisposeAsyncCore(); | ||
| } |
| public async Task EmptyPrompt_YieldsNothing() | ||
| { | ||
| using var client = new CopilotSdkChatClient(new CopilotSdkConfiguration()); | ||
| var count = 0; | ||
| await foreach (var _ in client.GetStreamingResponseAsync([new ChatMessage(ChatRole.User, "")])) | ||
| count++; | ||
| Assert.Equal(0, count); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task NoUserMessage_YieldsNothing() | ||
| { | ||
| using var client = new CopilotSdkChatClient(new CopilotSdkConfiguration()); | ||
| var count = 0; | ||
| await foreach (var _ in client.GetStreamingResponseAsync([new ChatMessage(ChatRole.System, "hi")])) | ||
| count++; | ||
| Assert.Equal(0, count); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ImplementsInterfaces() | ||
| { | ||
| using var client = new CopilotSdkChatClient(new CopilotSdkConfiguration()); | ||
| Assert.IsAssignableFrom<IChatClient>(client); | ||
| Assert.IsAssignableFrom<IAsyncDisposable>(client); | ||
| } | ||
| } | ||
|
|
||
| public class CopilotSdkDiRegistrationTests | ||
| { | ||
| [Fact] | ||
| public void AddCopilotSdkChatClient_RegistersIChatClient() | ||
| { | ||
| var services = new Microsoft.Extensions.DependencyInjection.ServiceCollection(); | ||
| services.AddCopilotSdkChatClient(); | ||
| var sp = services.BuildServiceProvider(); | ||
| var chatClient = sp.GetService<IChatClient>(); | ||
| Assert.NotNull(chatClient); | ||
| Assert.IsType<CopilotSdkChatClient>(chatClient); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void AddCopilotSdkChatClient_AppliesConfiguration() | ||
| { | ||
| var services = new Microsoft.Extensions.DependencyInjection.ServiceCollection(); | ||
| services.AddCopilotSdkChatClient(config => | ||
| { | ||
| config.Model = "claude-sonnet-4.5"; | ||
| config.CliPath = "/custom/path"; | ||
| }); | ||
| var sp = services.BuildServiceProvider(); | ||
| var config = sp.GetRequiredService<CopilotSdkConfiguration>(); | ||
| Assert.Equal("claude-sonnet-4.5", config.Model); | ||
| Assert.Equal("/custom/path", config.CliPath); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void AddCopilotSdkChatClient_RegistersConfiguration() | ||
| { | ||
| var services = new Microsoft.Extensions.DependencyInjection.ServiceCollection(); | ||
| services.AddCopilotSdkChatClient(); | ||
| var sp = services.BuildServiceProvider(); | ||
| var config = sp.GetService<CopilotSdkConfiguration>(); | ||
| Assert.NotNull(config); | ||
| } | ||
| } | ||
|
|
||
| public class CopilotSdkChatClientAdditionalUnitTests | ||
| { | ||
| [Fact] | ||
| public void StreamingTimeout_CanBeCustomized() | ||
| { | ||
| using var client = new CopilotSdkChatClient(new CopilotSdkConfiguration()); | ||
| client.StreamingTimeout = TimeSpan.FromSeconds(30); | ||
| Assert.Equal(TimeSpan.FromSeconds(30), client.StreamingTimeout); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetService_ReturnsCopilotClientType() | ||
| { | ||
| using var client = new CopilotSdkChatClient(new CopilotSdkConfiguration()); | ||
| // Before first call, both should be null | ||
| Assert.Null(client.GetService(typeof(GitHub.Copilot.SDK.CopilotClient))); | ||
| Assert.Null(client.GetService(typeof(GitHub.Copilot.SDK.CopilotSession))); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetService_UnknownType_ReturnsNull() | ||
| { | ||
| using var client = new CopilotSdkChatClient(new CopilotSdkConfiguration()); | ||
| Assert.Null(client.GetService(typeof(string))); | ||
| Assert.Null(client.GetService(typeof(int))); | ||
| Assert.Null(client.GetService(typeof(IDisposable))); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task EmptyToolsList_DoesNotCrash() | ||
| { | ||
| using var client = new CopilotSdkChatClient(new CopilotSdkConfiguration()); | ||
| var count = 0; | ||
| // Empty prompt with empty tools should yield nothing (short-circuits before SDK call) | ||
| await foreach (var _ in client.GetStreamingResponseAsync( | ||
| [new ChatMessage(ChatRole.User, "")], | ||
| new ChatOptions { Tools = [] })) | ||
| count++; | ||
| Assert.Equal(0, count); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task NullOptions_DoesNotCrash() | ||
| { | ||
| using var client = new CopilotSdkChatClient(new CopilotSdkConfiguration()); | ||
| var count = 0; | ||
| await foreach (var _ in client.GetStreamingResponseAsync( | ||
| [new ChatMessage(ChatRole.User, "")], | ||
| null)) | ||
| count++; | ||
| Assert.Equal(0, count); | ||
| } |
| var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); | ||
| _ = Task.Run(async () => | ||
| { | ||
| while (!timeoutCts.Token.IsCancellationRequested) | ||
| { | ||
| await Task.Delay(TimeSpan.FromSeconds(5), timeoutCts.Token).ConfigureAwait(false); | ||
| if (DateTime.UtcNow - lastActivity > StreamingTimeout) | ||
| { | ||
| streamError = new TimeoutException( | ||
| $"No response from Copilot SDK within {StreamingTimeout.TotalSeconds}s."); | ||
| channel.Writer.TryComplete(streamError); | ||
| break; | ||
| } | ||
| } | ||
| }, timeoutCts.Token); |
| public async Task<IList<ModelInfo>> ListModelsAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| await EnsureClientAsync(cancellationToken).ConfigureAwait(false); | ||
| return await _client!.ListModelsAsync().ConfigureAwait(false); | ||
| } |
| { | ||
| config.Model = "gpt-4.1"; | ||
| config.UseLoggedInUser = true; | ||
| config.CliPath = "/opt/homebrew/bin/copilot"; |
| public ChatViewModel(IChatClient chatClient, IList<AITool> tools) | ||
| { | ||
| _chatClient = (CopilotSdkChatClient)chatClient; |
Expert Code Review — PR #278Methodology: 3 independent reviewers with adversarial consensus (+ 6 follow-up evaluations for 3 disputed findings) 13 findings posted as inline comments — 5 🔴 critical, 6 🟡 moderate, 2 🟢 minor Overflow Findings
Discarded FindingsThe following were flagged by only 1 reviewer and did not reach consensus (either failed follow-up validation or were discarded below the 3-finding cap):
CI StatusSeveral CI checks are still in progress (macOS and Windows builds for CopilotSdk and other products). Ubuntu build passed ✅. License/CLA check passed ✅. Test CoverageThe PR includes both unit tests and integration tests in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Expert Code Review: 13 findings posted (5 critical, 6 moderate, 2 minor). See inline comments for details and the lean summary comment for the full overview.
Generated by Expert Code Review (auto) for issue #278 · ● 20.9M
| } | ||
| }); | ||
|
|
||
| var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); |
There was a problem hiding this comment.
🔴 CRITICAL · 3/3 consensus · CancellationTokenSource never disposed — resource leak on every streaming call
CreateLinkedTokenSource registers a callback on the caller's CancellationToken. This CTS is never disposed — Cancel() on line 207 doesn't release the registration. On error paths (SendAsync throws, channel read cancelled), even Cancel() is never reached.
Fix: using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
| var lastActivity = DateTime.UtcNow; | ||
| Exception? streamError = null; | ||
|
|
||
| using var sub = _session!.On(evt => |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 consensus · Concurrent GetStreamingResponseAsync calls receive each other's events
_initLock protects session initialization but not message sending. Two concurrent callers both register _session.On(...) subscriptions and both call _session.SendAsync(...). Events from both responses dispatch to both subscriptions, mixing streams.
Fix: Add a per-send SemaphoreSlim serializing SendAsync + channel drain, or document that the class is not safe for concurrent sends.
| _config = config; | ||
| } | ||
|
|
||
| public void Dispose() |
There was a problem hiding this comment.
🔴 CRITICAL · 2/3 consensus · _disposed check-and-set is not atomic — concurrent Dispose()/DisposeAsync() calls can both pass the guard
Two threads can both read _disposed == false, both proceed, and both call DisposeAsyncCore() — double-disposing the SemaphoreSlim and SDK objects.
Fix: Change _disposed to int and use if (Interlocked.Exchange(ref _disposed, 1) == 1) return;
| { | ||
| _client ??= new CopilotClient(new CopilotClientOptions | ||
| { |
There was a problem hiding this comment.
🔴 CRITICAL · 2/3 consensus · _client is assigned before StartAsync succeeds — permanently broken client on failure
_client ??= new CopilotClient(...) commits the field immediately. If StartAsync then throws (network error, bad credentials), _client is left pointing to a never-started instance. The early-exit guard if (_client is not null) return; on line 294 will skip re-initialization on all subsequent calls.
Fix: Construct into a local variable, await StartAsync, then assign to _client only on success:
if (_client is null)
{
var c = new CopilotClient(new CopilotClientOptions { ... });
await c.StartAsync(cancellationToken);
_client = c;
}| { | ||
| if (_disposed) return; | ||
| _disposed = true; | ||
| _ = DisposeAsyncCore(); |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 consensus · Sync Dispose() fire-and-forgets async disposal
_ = DisposeAsyncCore() discards the task. If _session.DisposeAsync() or _client.DisposeAsync() throws, it's silently swallowed. More importantly, _initLock.Dispose() runs asynchronously after Dispose() returns — any thread waiting on _initLock.WaitAsync() gets an ObjectDisposedException at an unpredictable time.
Fix: Either block synchronously (DisposeAsyncCore().GetAwaiter().GetResult()) or acquire _initLock before disposing resources.
|
|
||
| var lastUserMessage = messages.LastOrDefault(m => m.Role == ChatRole.User); |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 consensus · IChatClient contract: full conversation history silently discarded
Only messages.LastOrDefault(m => m.Role == ChatRole.User) is used. All prior turns, system messages, and non-last user messages are silently dropped. Callers migrating from other IChatClient backends (OpenAI, Anthropic) will silently lose conversation context. Additionally, image-only user messages (no text) are short-circuited before attachments are sent, breaking the advertised vision support.
Fix: Document this limitation explicitly in the XML summary and README. Consider logging a warning when messages.Count() > 1.
| { | ||
| if (_session is not null) return; | ||
|
|
||
| await EnsureClientAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
🔴 CRITICAL · 2/3 consensus · Deadlock: EnsureSessionAsync holds _initLock then calls EnsureClientAsync which tries to acquire the same SemaphoreSlim(1,1)
Since SemaphoreSlim is not reentrant, the first call to GetStreamingResponseAsync (when _client is null) will hang forever here. This affects every code path that needs both client and session initialization.
Fix: Inline the client-creation logic into this method (avoiding the nested lock acquisition), or split into a lock-assuming EnsureClientCoreAsync and a lock-taking wrapper for standalone callers like ListModelsAsync.
| ChatOptions? options = null, | ||
| [EnumeratorCancellation] CancellationToken cancellationToken = default) | ||
| { | ||
| await EnsureSessionAsync(options, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 consensus (via follow-up) · EnsureSessionAsync runs before the empty-prompt guard — wastes resources and breaks unit tests
EnsureSessionAsync (here) spawns the CLI process and creates a real backend session, but the empty-prompt check on lines 101-102 immediately yield breaks. In environments without Copilot CLI, this throws before the guard can return.
Fix: Move the prompt extraction and empty-prompt check before EnsureSessionAsync:
var lastUserMessage = messages.LastOrDefault(m => m.Role == ChatRole.User);
var prompt = lastUserMessage?.Text ?? "";
if (string.IsNullOrEmpty(prompt)) yield break;
await EnsureSessionAsync(options, cancellationToken).ConfigureAwait(false);| // Determine if we need to reset the session | ||
| var toolsChanged = _session is not null && !ReferenceEquals(tools, _sessionTools) && tools is { Count: > 0 }; |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 consensus · TOCTOU race: toolsChanged/modelChanged computed without holding _initLock
_session, _sessionTools, and _sessionModel are read at lines 320-321 without the lock. A concurrent ResetSessionAsync() (which holds the lock) can null all three simultaneously, producing a torn/stale state for these comparisons. Also, the logic only detects tool additions (tools is { Count: > 0 }) — removing tools (passing null/empty) won't trigger a session reset.
Fix: Move the change detection inside the lock, or accept/document the benign race. Extend detection to handle tool removal.
| }); | ||
|
|
||
| var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); | ||
| _ = Task.Run(async () => |
There was a problem hiding this comment.
🟢 MINOR · 2/3 consensus · Timeout background task's OperationCanceledException is unobserved
When timeoutCts is cancelled, Task.Delay throws TaskCanceledException which faults the fire-and-forget Task.Run. This surfaces as a spurious failure in environments with TaskScheduler.UnobservedTaskException handlers.
Fix: Wrap the lambda body in try/catch (OperationCanceledException) { }, or use PeriodicTimer with explicit cancellation.
- Add CliUrl config for connecting to external Copilot CLI server (needed for Mac Catalyst which can't fork child processes) - Add AutoStart=false when CliUrl is set to skip process spawning - Wrap SDK StartAsync in dedicated Thread + WhenAny for 15s timeout - Run streaming loop in Task.Run to escape main-thread SyncContext - Dispatch all UI updates via MainThread.InvokeOnMainThreadAsync - Add StatusText label showing connection/streaming state - Known issue: Mac Catalyst still hangs during SDK StartAsync even with CliUrl — the SDK likely does process validation before connecting. Works perfectly as a .NET 10 console app. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Revert CliUrl/UseStdio/AutoStart/Thread workarounds — the working copilot-chat-control branch uses a simple CliPath + direct StartAsync. - Remove CliUrl config (not needed for this SDK version) - Use simple await StartAsync like the working branch - Restore proper entitlements (sandbox=false + network client/server) - Remove CodesignEntitlements blank override from csproj - Auto-discover SDK cached CLI binary in ~/Library/Caches/copilot-sdk-* Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation
The GitHub Copilot SDK (
GitHub.Copilot.SDKNuGet package) provides a .NET client for interacting with Copilot, but it uses its own session/event API rather than the standardMicrosoft.Extensions.AI.IChatClientinterface. This PR adds a new product that bridges the two, letting any code that works withIChatClientuse the Copilot SDK as a backend -- streaming, tool calling, model selection, and all.What's included
Microsoft.Maui.CopilotSdklibrary (src/CopilotSdk/)An
IChatClientadapter wrappingCopilotClient/CopilotSessionwith:AssistantMessageDeltaEvent, etc.) toIAsyncEnumerable<ChatResponseUpdate>viaChannel<T>ChatOptions.Toolsto the SDK session natively; surfacesFunctionCallContent/FunctionResultContentin the streamChatOptions.ModelIdoverrides the configured model per-request (resets session automatically)ChatOptions.ResponseFormat = Jsoninjects a JSON-only system message instructionAssistantReasoningDeltaEventsurfaced asTextContentwithreasoning=truemetadata for chain-of-thought visibilityChatOptions.AdditionalProperties["ReasoningEffort"]DataContentwith image MIME types mapped to SDK blob attachmentsListModelsAsync()-- public method exposing the SDK's model discoveryAddCopilotSdkChatClient()extension methodSemaphoreSlim-based lazy init,IAsyncDisposableSample app (
samples/CopilotSdkSample/)A .NET MAUI app serving as a playground for the adapter features:
Tests (
tests/CopilotSdk/)Build infrastructure
GitHub.Copilot.SDK0.3.0 added to CPM (Directory.Packages.props,eng/Versions.props)GitHub.Copilot.SDK.dllineng/Signing.propssrc/CopilotSdk/CopilotSdk.slnf.github/workflows/ci-copilotsdk.ymlMauiLabs.slnxDocumented limitations
Temperature, MaxOutputTokens, StopSequences, FrequencyPenalty, PresencePenalty, TopP, and Seed are not supported by the Copilot SDK natively. These
ChatOptionsproperties are ignored. This is documented in the XML docs onCopilotSdkChatClient.