[AI] Windows Copilot Runtime (Phi Silica) support#178
Conversation
There was a problem hiding this comment.
Expert Code Review — PR #178: Windows Copilot Runtime (Phi Silica) Support
Reviewed all changed files across the library, sample, and device tests. Below are the findings ordered by severity.
🔴 CRITICAL
1. DI crash on Windows: ISemanticSearchService resolution throws at runtime
- File:
samples/Microsoft.Maui.Essentials.AI.Sample/MauiProgram.cs - Line: 70–71
- Scenario: On Windows,
AddPhiSilicaServices()registersISemanticSearchService→AppContentIndexerSearchService(line 205). Then line 70 unconditionally overwrites it with a factory that resolvesIEmbeddingGenerator<string, Embedding<float>>— which is never registered in the Windows path. The last registration wins in DI, so any consumer ofISemanticSearchServicewill throwInvalidOperationExceptionat runtime. - Recommendation: Wrap lines 69–71 in
#if !WINDOWS(or move the registration into each platform method that actually registers an embedding generator), so on Windows onlyAppContentIndexerSearchServiceis used.
🟡 MODERATE
2. Negative TopK silently wraps to huge uint — unexpected model behavior
- File:
src/AI/Microsoft.Maui.Essentials.AI/Platform/Windows/PhiSilicaChatClient.cs - Line: 249
- Scenario:
ChatOptions.TopKisint. If a consumer passes a negative value (e.g., -1 as sentinel),(uint)topKwraps to4294967295, causing the model to behave unpredictably. - Recommendation: Add a guard:
if (topK < 0) throw new ArgumentOutOfRangeException(nameof(options), "TopK must be non-negative."); languageModelOptions.TopK = (uint)topK;
3. CancellationTokenRegistration not disposed — potential memory leak with long-lived tokens
- File:
src/AI/Microsoft.Maui.Essentials.AI/Platform/Windows/PhiSilicaChatClient.cs - Line: 122
- Scenario:
cancellationToken.Register(...)returns aCancellationTokenRegistration. If the caller uses a long-lived or linked token, the registration (and its closure capturingoperation) stays alive until the token is disposed. For short-lived per-request tokens this is fine; for singleton tokens it leaks. - Recommendation: Capture the registration and dispose it after the enumeration completes:
var ctr = cancellationToken.Register(() => operation.Cancel()); try { await foreach (...) { yield return update; } } finally { ctr.Dispose(); }
4. Thread-unsafe Dictionary access in AppContentIndexerSearchService
- File:
samples/Microsoft.Maui.Essentials.AI.Sample/Services/AppContentIndexerSearchService.cs - Lines: 15–30, 34, 44
- Scenario:
GetOrCreateIndexerreads/writes a plainDictionary<string, AppContentIndexer>from multipleTask.Runcallbacks concurrently (bothIndexAsyncandSearchAsynccall it from pool threads). Concurrent access toDictionarycauses corruption orKeyNotFoundException. - Recommendation: Use
ConcurrentDictionary<string, AppContentIndexer>withGetOrAdd, or add a lock around the method.
5. PhiSilicaExtensions.AsIChatClient missing [SupportedOSPlatform] attribute
- File:
src/AI/Microsoft.Maui.Essentials.AI/Platform/Windows/PhiSilicaExtensions.cs - Lines: 10, 23
- Scenario:
PhiSilicaChatClientis annotated with[SupportedOSPlatform("windows10.0.26100.0")], but the extension methodAsIChatClient(and its class) has no such attribute. Consumers callingmodel.AsIChatClient()won't get platform compatibility warnings at their call-site. - Recommendation: Add
[SupportedOSPlatform("windows10.0.26100.0")]to the class or method.
🟢 MINOR
6. JsonDocument not disposed in BuildToolCallSchema
- File:
samples/Microsoft.Maui.Essentials.AI.Sample/Services/PhiSilicaToolsAndSchemaClient.cs - Lines: 271, 293
- Scenario:
JsonDocument.Parse(json).RootElement.Clone()creates aJsonDocumentthat is never disposed. While.Clone()makes the element independent and the GC will clean up eventually, it pins pooled buffers until finalization. - Recommendation:
using var doc = JsonDocument.Parse(json); return doc.RootElement.Clone();
7. Potential empty-prompt edge case when options.Instructions is set but all messages have only non-text content
- File:
src/AI/Microsoft.Maui.Essentials.AI/Platform/Windows/PhiSilicaChatClient.cs - Lines: 78–82
- Scenario: When
options.Instructionsis provided,NormalizeChatMessagesreturns it as system prompt and keeps all messages (including system role messages) in history. If the only message is a system message (now used as both Instructions context and still in history), theConvertToPromptwill include aSystem: ...prefix which may confuse the model since it's already in the context. - Recommendation: When
options.Instructionsis set, consider filtering out the original system message from history if it duplicates the instructions, or document thatInstructionstakes precedence and system messages are preserved as-is.
8. Silent exception swallowing in tool enum extraction
- File:
samples/Microsoft.Maui.Essentials.AI.Sample/Services/PhiSilicaToolsAndSchemaClient.cs - Line: 206
- Scenario:
catch { }silently swallows all exceptions from JSON parsing of tool schemas. If a tool schema is malformed, the user gets no diagnostic feedback. - Recommendation: At minimum log a warning or Debug.Write so developers can troubleshoot schema issues.
Summary
| Severity | Count |
|---|---|
| 🔴 CRITICAL | 1 |
| 🟡 MODERATE | 4 |
| 🟢 MINOR | 3 |
The critical DI issue (#1) will cause a runtime crash for any Windows user of the sample app when ISemanticSearchService is resolved. The moderate issues are correctness/safety concerns in the shipped library code. The minor issues are in sample code and represent best-practice gaps.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
learn.microsoft.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "learn.microsoft.com"See Network Configuration for more information.
Generated by Expert Code Review (auto) for issue #178 · ● 24.1M
- PhiSilicaChatClient: save CancellationTokenRegistration + try/finally with operation.Cancel()+registration.Dispose() for early streaming exit - PhiSilicaChatClient: fix Dispose() to chain ContinueWith for in-flight _modelTask to avoid leaking LanguageModel COM object - PhiSilicaToolsAndSchemaClient: catch (JsonException) instead of bare catch - PhiSilicaToolsAndSchemaClient: use 'using var' for JsonDocument instances - AppContentIndexerSearchService: add _indexersLock + lock in GetOrCreateIndexer, WaitUntilReadyAsync, and Dispose for thread safety - MauiProgram: guard EmbeddingSearchService registration in #if !WINDOWS to prevent DI override of Windows AppContentIndexerSearchService Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- PhiSilicaChatClient: save CancellationTokenRegistration + try/finally with operation.Cancel()+registration.Dispose() for early streaming exit - PhiSilicaChatClient: fix Dispose() to chain ContinueWith for in-flight _modelTask to avoid leaking LanguageModel COM object - PhiSilicaToolsAndSchemaClient: catch (JsonException) instead of bare catch - PhiSilicaToolsAndSchemaClient: use 'using var' for JsonDocument instances - AppContentIndexerSearchService: add _indexersLock + lock in GetOrCreateIndexer, WaitUntilReadyAsync, and Dispose for thread safety - MauiProgram: guard EmbeddingSearchService registration in #if !WINDOWS to prevent DI override of Windows AppContentIndexerSearchService Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
de9cc86 to
06352f5
Compare
- PhiSilicaChatClient: save CancellationTokenRegistration + try/finally with operation.Cancel()+registration.Dispose() for early streaming exit - PhiSilicaChatClient: fix Dispose() to chain ContinueWith for in-flight _modelTask to avoid leaking LanguageModel COM object - PhiSilicaToolsAndSchemaClient: catch (JsonException) instead of bare catch - PhiSilicaToolsAndSchemaClient: use 'using var' for JsonDocument instances - AppContentIndexerSearchService: add _indexersLock + lock in GetOrCreateIndexer, WaitUntilReadyAsync, and Dispose for thread safety - MauiProgram: guard EmbeddingSearchService registration in #if !WINDOWS to prevent DI override of Windows AppContentIndexerSearchService Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
06352f5 to
56bb204
Compare
- Add PhiSilicaChatClient, PhiSilicaExtensions, PhiSilicaModelFactory - Add Windows test files: PhiSilicaChatClientTests, PhiSilicaExperimentTests - Mark 3 base test methods virtual for Phi Silica overrides - Add AppContentIndexerSearchService + PhiSilicaToolsAndSchemaClient to sample - Register AddPhiSilicaServices in MauiProgram.cs (#elif WINDOWS) - Update Windows Package.appxmanifest: systemAIModels capability, MaxVersionTested=10.0.26226.0 - Add Microsoft.WindowsAppSDK VersionOverride=2.0.0-experimental6 (AI projects only) - Add AppxOSMaxVersionTestedReplaceManifestVersion=false to prevent SDK override - Add SelfContained + WindowsAppSDKSelfContained for DeviceTests Windows build - Update PublicAPI/net-windows/PublicAPI.Unshipped.txt with 7 new API entries - Move TOOL-CALLING-README.md to docs/ai/ - Add src/AI/Directory.Build.props (chained import only) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- PhiSilicaChatClient: save CancellationTokenRegistration + try/finally with operation.Cancel()+registration.Dispose() for early streaming exit - PhiSilicaChatClient: fix Dispose() to chain ContinueWith for in-flight _modelTask to avoid leaking LanguageModel COM object - PhiSilicaToolsAndSchemaClient: catch (JsonException) instead of bare catch - PhiSilicaToolsAndSchemaClient: use 'using var' for JsonDocument instances - AppContentIndexerSearchService: add _indexersLock + lock in GetOrCreateIndexer, WaitUntilReadyAsync, and Dispose for thread safety - MauiProgram: guard EmbeddingSearchService registration in #if !WINDOWS to prevent DI override of Windows AppContentIndexerSearchService Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add Microsoft.Windows.SDK.BuildTools.WinApp to sample and device tests to enable dotnet run for packaged MSIX apps via winapp CLI - Remove WindowsPackageType=None from sample (was running unpackaged, blocking systemAIModels capability and GetReadyState) - Add SelfContained/WindowsAppSDKSelfContained to sample for Windows - Add MODE_XHARNESS DefineConstants when TestingMode=XHarness - Add temporary arg-passing proof-of-concept in App.xaml.cs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…mple Update namespace in Windows-specific service files and device tests to match the new sample project name (PR #171 rename). - AppContentIndexerSearchService.cs: Maui.Controls.Sample.Services -> EssentialsAISample.Services - PhiSilicaToolsAndSchemaClient.cs: Maui.Controls.Sample.Services -> EssentialsAISample.Services - PhiSilicaChatClientTests.cs: using -> EssentialsAISample.Services - App.xaml.cs: Remove temporary arg-passing test code Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The experimental Microsoft.WindowsAppSDK 2.0.0-experimental6 package
and its transitive dependencies (Microsoft.WindowsAppSDK.Runtime,
Microsoft.Windows.AI.MachineLearning) are not available in the dnceng
NuGet feeds on macOS. Use the same IsOSPlatform('windows') conditional
pattern as the sample and test projects to exclude the Windows TFM
when building on non-Windows platforms.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Microsoft.WindowsAppSDK 2.0.0-experimental6 has a transitive dependency on Microsoft.WindowsAppSDK.Search which is only available on nuget.org, not in the dotnet-public AzDO upstream mirror. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e91177c to
b4c0008
Compare
…nerability Upgrade Microsoft.Agents.AI from 1.0.0-rc2 to 1.5.0 (stable) and Microsoft.Agents.AI.Hosting from 1.0.0-preview to 1.5.0-preview to resolve NU1902: transitive OpenTelemetry.Api 1.13.1 has a known moderate vulnerability (GHSA-g94r-2vxg-569j). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c9abd4c to
428e9dd
Compare
- Microsoft.Extensions.DependencyInjection: 10.0.5 -> 10.0.6 - Microsoft.Extensions.Logging.Abstractions: 10.0.5 -> 10.0.6 - Microsoft.Extensions.AI: 10.3.0 -> 10.5.2 - Microsoft.Extensions.AI.Abstractions: 10.3.0 -> 10.5.2 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Stacked on top of #171. Migrates Windows Phi Silica (Copilot Runtime) support from dotnet/maui#33605 into maui-labs.
Changes
Library (
src/AI/Microsoft.Maui.Essentials.AI)PhiSilicaChatClient,PhiSilicaExtensions,PhiSilicaModelFactory(verbatim copy)VersionOverride=2.0.0-experimental6forMicrosoft.WindowsAppSDK(AI projects only — does not affect other repo consumers)AppxOSMaxVersionTestedReplaceManifestVersion=falseto prevent SDK from overriding the manifest'sMaxVersionTestedPublicAPI/net-windows/PublicAPI.Unshipped.txtwith 7 new API entriesSample
AppContentIndexerSearchService(Windows semantic search via OS AppContentIndexer)PhiSilicaToolsAndSchemaClient(function-calling middleware for Phi Silica)AddPhiSilicaServicesinMauiProgram.csvia#elif WINDOWSPackage.appxmanifest:systemAIModelscapability +MaxVersionTested=10.0.26226.0DeviceTests
PhiSilicaChatClientTestsandPhiSilicaExperimentTests(verbatim,[Category]stripped)virtualso Phi Silica subclass can override themPhiSilicaToolsAndSchemaClientfrom sample (file link, Windows only)SelfContained+WindowsAppSDKSelfContainedfor Windows to bundle experimental WinAppSDK into MSIXDocs
TOOL-CALLING-README.mdtodocs/ai/(was in Platform/Windows in source)Notes
Microsoft.WindowsAppSDK 2.0.0-experimental6is required forMicrosoft.Windows.AI.Text.LanguageModel(Phi Silica WinRT API). TheVersionOverridescopes this to AI projects only.[SupportedOSPlatform]); TFM stays at19041for build compat.main.