[Essentials] WebAuthenticator: Centralize request lifecycle handling - #37426
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37426Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37426" |
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
Skill Validation Results
❌ Skill Validation Results —
|
5adbaaf to
568eea3
Compare
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 5 findings
See inline comments for details.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions?
568eea3 to
378e685
Compare
|
@kubaflo Thank you for asking me to review the AI suggestions. I addressed the Android non-default HTTPS port regression and lifecycle mismatch pass-through, and documented/tested the intentional decisions around strict route matching, built-in-first handling, and the persistent Windows |
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 9 findings
See inline comments for details.
|
Thanks @kubaflo for the follow-up and for triggering the additional review. The final follow-up commit, The final refactoring removed the trivial Android/Windows exception factories and their test-only coverage; the resulting diagnostics-only patch is four files with no public API, matching, lifecycle, fallback, cleanup, registration, or ownership changes. Validation remains green for the affected areas: Essentials unit tests 513/513, Android DeviceTests 381/381, Windows WebAuthenticator tests 28/28, Apple compile-only, Essentials/PublicAPI, formatter, and diff checks. I replied to all eight new findings and resolved their threads; two were addressed in code and six retain the intentional documented behavior. Thanks again for the detailed pass. |
|
/azp run |
|
/review -b improved-reviewer -p windows |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 6 findings
See inline comments for details.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions?
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 10 findings
See inline comments for details.
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@IlGalvo — new AI review results are available based on commit
8bdb794.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ✅ PASSED
Platform: WINDOWS · Base: net11.0 · Merge base: ee4d06cd
✅ Verified (new API / feature) — this PR adds new API and a test that references it in the same project, so reverting the fix un-compiles the test: there is no valid "fails without the fix" baseline to establish (a compile-coupled baseline). The gate instead verified the fix by a clean build + pass with the fix, so this is a real PASS rather than a non-committal INCONCLUSIVE.
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🧪 WebAuthenticatorRequestManager_Tests WebAuthenticatorRequestManager_Tests |
🛠️ BUILD ERROR | ✅ PASS — 11s |
🧪 WebUtils_Tests WebUtils_Tests |
🛠️ BUILD ERROR | ✅ PASS — 5s |
📱 WebAuthenticator_Windows_Tests (PlatformValidationPrecedesPreCanceledToken, ManifestProtocolLookupUsesCurrentApplicationAndVersionAgnosticUapNamespace, ManifestProtocolLookupCanUseTheOnlyApplicationAsIdentityFallback, RegistryOwnershipRejectsOnlyCertainOtherExecutables, RegistryOwnershipAcceptsDotnetHostedCommand, CallbackRouteExceptionsPreserveTheRedactedCauseChain) Category=WebAuthenticator |
🛠️ BUILD ERROR | ✅ PASS — 214s |
🔴 Without fix — 🧪 WebAuthenticatorRequestManager_Tests: 🛠️ BUILD ERROR · 26s
Error-relevant lines (filtered from the build log):
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(476,3): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(496,14): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(20,17): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(506,20): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(508,41): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(515,13): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
🟢 With fix — 🧪 WebAuthenticatorRequestManager_Tests: PASS ✅ · 11s
(no coded error found; showing last 1200 chars)
ngRequest [< 1 ms]
Passed Tests.WebAuthenticatorRequestManager_Tests.CallerCancellationConsumesOnlyMatchingDuplicateCallback [1 ms]
[xUnit.net 00:00:00.45] Finished: Microsoft.Maui.Essentials.UnitTests
Passed Tests.WebAuthenticatorRequestManager_Tests.OptionsAreSnapshottedBeforeConcurrentMutation [6 ms]
Passed Tests.WebAuthenticatorRequestManager_Tests.CancelByIdIsIdentitySafe [< 1 ms]
Passed Tests.WebAuthenticatorRequestManager_Tests.FailByIdIsIdentitySafe [< 1 ms]
Passed Tests.WebAuthenticatorRequestManager_Tests.DuplicateRouteCallbackIsConsumedWithoutRepeatingCompletion [< 1 ms]
Passed Tests.WebAuthenticatorRequestManager_Tests.BeginRejectsASecondValidRequest [< 1 ms]
Passed Tests.WebAuthenticatorRequestManager_Tests.DecoderRunsOutsideTheManagerLock [< 1 ms]
Passed Tests.WebAuthenticatorRequestManager_Tests.RouteOnlyMismatchIsUnhandledAndRemainsPending [< 1 ms]
Passed Tests.WebAuthenticatorRequestManager_Tests.PreCanceledTokenIsPreservedAndDoesNotLaunch [< 1 ms]
Passed Tests.WebAuthenticatorRequestManager_Tests.InvalidSecondCallKeepsValidationPrecedence [1 ms]
Test Run Successful.
Total tests: 27
Passed: 27
Total time: 1.0185 Seconds
🔴 Without fix — 🧪 WebUtils_Tests: 🛠️ BUILD ERROR · 7s
Error-relevant lines (filtered from the build log):
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(476,3): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(496,14): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(20,17): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(506,20): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(508,41): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(515,13): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Essentials\test\UnitTests\Essentials.UnitTests.csproj]
🟢 With fix — 🧪 WebUtils_Tests: PASS ✅ · 5s
(no coded error found; showing last 1200 chars)
urnsExpected(expectedUrl: "https://example.com:8443/callback", callbackUrl: "https://example.com:9443/callback", expected: False) [< 1 ms]
Passed Tests.WebUtils_Tests.CanHandleCallback_ReturnsExpected(expectedUrl: "maui-auth:/callback%20complete", callbackUrl: "maui-auth:/callback%20complete?code=123", expected: True) [< 1 ms]
[xUnit.net 00:00:00.31] Finished: Microsoft.Maui.Essentials.UnitTests
Passed Tests.WebUtils_Tests.CanHandleCallback_ReturnsExpected(expectedUrl: "maui-auth://callback", callbackUrl: "MAUI-AUTH://CALLBACK?code=123", expected: True) [< 1 ms]
Passed Tests.WebUtils_Tests.ResolveRelativePath_EncodedDotDot_HandledCorrectly [3 ms]
Passed Tests.WebUtils_Tests.ResolveRelativePath_RootRequest_ReturnsEmpty [< 1 ms]
Passed Tests.WebUtils_Tests.ResolveRelativePath_DifferentOrigin_ReturnsNull [< 1 ms]
Passed Tests.WebUtils_Tests.ResolveRelativePath_DoubleSlash_MakeRelativeUri_ProducesRooted_ReturnsNull [< 1 ms]
Passed Tests.WebUtils_Tests.CanHandleCallback_RejectsRelativeUris [< 1 ms]
Passed Tests.WebUtils_Tests.ResolveRelativePath_SubPath_ReturnsPath [< 1 ms]
Test Run Successful.
Total tests: 34
Passed: 34
Total time: 0.7255 Seconds
🔴 Without fix — 📱 WebAuthenticator_Windows_Tests (PlatformValidationPrecedesPreCanceledToken, ManifestProtocolLookupUsesCurrentApplicationAndVersionAgnosticUapNamespace, ManifestProtocolLookupCanUseTheOnlyApplicationAsIdentityFallback, RegistryOwnershipRejectsOnlyCertainOtherExecutables, RegistryOwnershipAcceptsDotnetHostedCommand, CallbackRouteExceptionsPreserveTheRedactedCauseChain): 🛠️ BUILD ERROR · 292s
Error-relevant lines (filtered from the build log):
D:\a\1\s\src\Essentials\test\DeviceTests\Tests\Windows\WebAuthenticator_Windows_Tests.cs(95,47): error CS0117: 'WebAuthenticatorImplementation' does not contain a definition for 'IsUriProtocolDeclared' [D:\a\1\s\src\Essentials\test\DeviceTests\Essentials.DeviceTests.csproj::TargetFramework=net11.0-windows10.0.19041.0]
D:\a\1\s\src\Essentials\test\DeviceTests\Tests\Windows\WebAuthenticator_Windows_Tests.cs(96,48): error CS0117: 'WebAuthenticatorImplementation' does not contain a definition for 'IsUriProtocolDeclared' [D:\a\1\s\src\Essentials\test\DeviceTests\Essentials.DeviceTests.csproj::TargetFramework=net11.0-windows10.0.19041.0]
D:\a\1\s\src\Essentials\test\DeviceTests\Tests\Windows\WebAuthenticator_Windows_Tests.cs(97,48): error CS0117: 'WebAuthenticatorImplementation' does not contain a definition for 'IsUriProtocolDeclared' [D:\a\1\s\src\Essentials\test\DeviceTests\Essentials.DeviceTests.csproj::TargetFramework=net11.0-windows10.0.19041.0]
D:\a\1\s\src\Essentials\test\DeviceTests\Tests\Windows\WebAuthenticator_Windows_Tests.cs(118,47): error CS0117: 'WebAuthenticatorImplementation' does not contain a definition for 'IsUriProtocolDeclared' [D:\a\1\s\src\Essentials\test\DeviceTests\Essentials.DeviceTests.csproj::TargetFramework=net11.0-windows10.0.19041.0]
D:\a\1\s\src\Essentials\test\DeviceTests\Tests\Windows\WebAuthenticator_Windows_Tests.cs(133,48): error CS0117: 'WebAuthenticatorImplementation' does not contain a definition for 'IsRegistryCommandOwnedByCurrentExecutable' [D:\a\1\s\src\Essentials\test\DeviceTests\Essentials.DeviceTests.csproj::TargetFramework=net11.0-windows10.0.19041.0]
D:\a\1\s\src\Essentials\test\DeviceTests\Tests\Windows\WebAuthenticator_Windows_Tests.cs(143,48): error CS0117: 'WebAuthenticatorImplementation' does not contain a definition for 'IsRegistryCommandOwnedByCurrentExecutable' [D:\a\1\s\src\Essentials\test\DeviceTests\Essentials.DeviceTests.csproj::TargetFramework=net11.0-windows10.0.19041.0]
D:\a\1\s\src\Essentials\test\DeviceTests\Tests\Windows\WebAuthenticator_Windows_Tests.cs(154,59): error CS0117: 'WebAuthenticatorImplementation' does not contain a definition for 'CreateCallbackRouteException' [D:\a\1\s\src\Essentials\test\DeviceTests\Essentials.DeviceTests.csproj::TargetFramework=net11.0-windows10.0.19041.0]
D:\a\1\s\src\Essentials\test\DeviceTests\Tests\Windows\WebAuthenticator_Windows_Tests.cs(157,54): error CS0117: 'WebAuthenticatorImplementation' does not contain a definition for 'CreateCallbackRouteException' [D:\a\1\s\src\Essentials\test\DeviceTests\Essentials.DeviceTests.csproj::TargetFramework=net11.0-windows10.0.19041.0]
Build FAILED.
🟢 With fix — 📱 WebAuthenticator_Windows_Tests (PlatformValidationPrecedesPreCanceledToken, ManifestProtocolLookupUsesCurrentApplicationAndVersionAgnosticUapNamespace, ManifestProtocolLookupCanUseTheOnlyApplicationAsIdentityFallback, RegistryOwnershipRejectsOnlyCertainOtherExecutables, RegistryOwnershipAcceptsDotnetHostedCommand, CallbackRouteExceptionsPreserveTheRedactedCauseChain): PASS ✅ · 214s
(no coded error found; showing last 1200 chars)
press the warning by specifying `x:SuppressXamlTrimWarnings=True` within the closest element. If not, the property path might be trimmed and will not be AOT compatible. [D:\a\1\s\src\Controls\src\Core\Controls.Core.csproj::TargetFramework=net11.0-windows10.0.19041.0]
Platform\Windows\TabbedPage\TabbedPageStyle.xaml(19,21): XamlCompiler warning WMC1510: Ensure the property path is trimming and AOT compatible by making use of 'Compiled Bindings (x:bind)' if possible or by specifying the 'x:DataType' directive with the respective binding data context and marking the type declaration with the 'WinRT.GeneratedBindableCustomProperty' attribute or the 'Microsoft.UI.Xaml.Data.Bindable' attribute. If you are unable to do either but can ensure the data type is attributed correctly, then you can also suppress the warning by specifying `x:SuppressXamlTrimWarnings=True` within the closest element. If not, the property path might be trimmed and will not be AOT compatible. [D:\a\1\s\src\Controls\src\Core\Controls.Core.csproj::TargetFramework=net11.0-windows10.0.19041.0]
0 Error(s)
Time Elapsed 00:03:23.61
Passed: 14
Failed: 0
Skipped: 0
Total: 14
Tests completed successfully
⚠️ Failure Details
- 🛠️ WebAuthenticatorRequestManager_Tests without fix: build failed before tests could run
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(476,3): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using di...
- 🛠️ WebUtils_Tests without fix: build failed before tests could run
D:\a\1\s\src\Essentials\test\UnitTests\WebAuthenticatorRequestManager_Tests.cs(476,3): error CS0246: The type or namespace name 'WebAuthenticatorRequest' could not be found (are you missing a using di...
- 🛠️ WebAuthenticator_Windows_Tests (PlatformValidationPrecedesPreCanceledToken, ManifestProtocolLookupUsesCurrentApplicationAndVersionAgnosticUapNamespace, ManifestProtocolLookupCanUseTheOnlyApplicationAsIdentityFallback, RegistryOwnershipRejectsOnlyCertainOtherExecutables, RegistryOwnershipAcceptsDotnetHostedCommand, CallbackRouteExceptionsPreserveTheRedactedCauseChain) without fix: build failed before tests could run
D:\a\1\s\src\Essentials\test\DeviceTests\Tests\Windows\WebAuthenticator_Windows_Tests.cs(95,47): error CS0117: 'WebAuthenticatorImplementation' does not contain a definition for 'IsUriProtocolDeclared...
📁 Fix files reverted (13 files)
src/Essentials/samples/Samples/Platforms/Android/MainActivity.cssrc/Essentials/samples/Samples/Platforms/MacCatalyst/Info.plistsrc/Essentials/samples/Samples/Platforms/Windows/Package.appxmanifestsrc/Essentials/samples/Samples/Platforms/iOS/Info.plistsrc/Essentials/samples/Samples/View/WebAuthenticatorPage.xamlsrc/Essentials/samples/Samples/ViewModel/WebAuthenticatorViewModel.cssrc/Essentials/src/Types/Shared/WebUtils.shared.cssrc/Essentials/src/WebAuthenticator/WebAuthenticator.android.cssrc/Essentials/src/WebAuthenticator/WebAuthenticator.ios.tvos.cssrc/Essentials/src/WebAuthenticator/WebAuthenticator.shared.cssrc/Essentials/src/WebAuthenticator/WebAuthenticator.windows.cssrc/Essentials/src/WebAuthenticator/WebAuthenticatorCallbackActivity.android.cssrc/Essentials/src/WebAuthenticator/WebAuthenticatorIntermediateActivity.android.cs
New files (not reverted):
src/Essentials/src/WebAuthenticator/WebAuthenticatorRequest.shared.cssrc/Essentials/src/WebAuthenticator/WebAuthenticatorRequestManager.shared.cs
📋 Pre-Flight — Context & Validation
PR #37426 Pre-Flight
Context
- PR:
[Essentials] WebAuthenticator: Centralize request lifecycle handling - Base:
net11.0 - Materialized review commit:
e1a1bb4963cc6287c3e9e45d2cd49b4272d48742 - Merge base:
ee4d06cde6b49e297631b08426a33fb34f3152ef - Issue fixed: #32766, where pressing Android Back from the Microsoft account chooser closes the authentication UI but leaves
AuthenticateAsyncpending indefinitely. - Platform available for this phase: Windows.
Diff Inspection
The squashed PR diff was inspected directly. It changes 22 files (+3015/-505), introducing a process-wide WebAuthenticatorRequestManager and identity-bearing WebAuthenticatorRequest, then routing Android, Apple, and Windows callback/cancellation/failure paths through that shared exactly-once completion mechanism. Android additionally uses a request-owned intermediate activity so returning from Custom Tab/system-browser authentication without a callback cancels the matching request. The PR also tightens callback route matching, preserves custom authenticator fallback, changes Windows AppInstance route ownership, updates samples/docs, and adds unit/device tests.
The central existing approach is therefore global identity-safe request ownership plus platform adapters, with Android Back cancellation detected through an intermediate activity. Alternatives must use a different root-cause mechanism rather than restating this manager/intermediate-activity design.
Gate Evidence
The prior gate passed and must not be rerun. It detected:
- Primary unit test:
dotnet test src/Essentials/test/UnitTests/Essentials.UnitTests.csproj --filter "FullyQualifiedName~WebAuthenticatorRequestManager_Tests" - Mandatory URI-matching regression test:
dotnet test src/Essentials/test/UnitTests/Essentials.UnitTests.csproj --filter "FullyQualifiedName~WebUtils_Tests" - Mandatory Windows regression test:
pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Essentials -Platform windows -TestFilter "Category=WebAuthenticator"
With the PR fix, the focused unit tests and 14 Windows WebAuthenticator device tests passed. The without-fix side is compile-coupled because the PR adds request-manager production files and tests together.
Try-Fix Boundaries
- Use one implementation/test pass and at most one focused correction/retest.
- Do not run a full suite or repeat gate verification.
EstablishBrokenBaseline.ps1previously classified these PR production files as newly added:src/Essentials/src/WebAuthenticator/WebAuthenticatorRequest.shared.cssrc/Essentials/src/WebAuthenticator/WebAuthenticatorRequestManager.shared.cs
- The loaded
try-fixskill requires an attempt to report Blocked before editing whenever.github/.baseline-state.jsonhas a non-emptyNewFilesarray. Do not bypass that safety rule. - The worktree contains many pre-existing unrelated
.github/engchanges. Preserve them exactly; use onlypwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restorefor attempt restoration. - Do not create or overwrite
gate/content.md.
🔬 Code Review — Deep Analysis
PR #37426 — Initial Expert Evaluation (Phase 1)
- PR:
[Essentials] WebAuthenticator: Centralize request lifecycle handling - Reviewed commit:
e1a1bb4963cc6287c3e9e45d2cd49b4272d48742(raw submitted PR inD:\a\1\s) - Merge base:
ee4d06cde6b49e297631b08426a33fb34f3152ef(basenet11.0) - Issue: #32766 — Android Back from the account chooser closes the auth UI but leaves
AuthenticateAsyncpending forever. - Gate (trusted, not rerun): PASSED — tests fail without fix, pass with fix.
- Scope of this document: initial evaluation of the submitted PR only. No try-fix or pr-plus-reviewer candidate was reviewed.
Verdict
Approve with changes — the architecture is right and the root cause is genuinely addressed, but three classes of issue should be resolved before merge: a process-wide request-slot leak window on all three platforms, an undocumented-in-practice concurrency behavior break, and two Windows validation guards that were made strictly more restrictive and now reject previously working configurations.
Confidence: Medium-High. The diff was read in full for all production files and the mechanism was traced end-to-end on Android, iOS/Mac Catalyst, and Windows. Confidence is not "High" because the Android Auth Tab path (AndroidX.Browser.Auth) and the Windows unpackaged registry path cannot be exercised from this Windows-only review environment, and the gate evidence covers Windows device tests plus shared unit tests only.
Independent Assessment (formed before reading pre-flight/try-fix narrative)
The diff replaces per-implementation mutable ambient state (tcsResponse, currentRedirectUri, currentOptions, currentAppWindow, currentViewController, was/sf) with:
WebAuthenticatorRequest— an immutable, identity-bearing request object holding its ownTaskCompletionSource(RunContinuationsAsynchronously) plus aCompletionClaimedflag guarded by the manager's lock.WebAuthenticatorRequestManager— a process-wide single-slot owner providing exactly-once completion (TryReserveunder lock, then complete outside the lock), route matching viaWebUtils.CanHandleCallback, and explicitBegin/Endlifetime.- Platform adapters that no longer own completion: Android routes through a rewritten
WebAuthenticatorIntermediateActivity(modes: AuthTab / CustomTab / Callback / Cleanup / Browser) with a request id carried in intent extras and restored inOnSaveInstanceState; iOS/Mac Catalyst always usesASWebAuthenticationSession(legacySFAuthenticationSession/SFSafariViewControllerpaths deleted) with completion marshalled throughDispatchQueue.MainQueue.DispatchAsync; Windows keepsAppInstanceroute-key ownership but delegates completion to the manager.
Root cause fit. The reported hang is that returning from the browser/Custom Tab without a callback produced no observation point. The fix's OnResume path in the intermediate activity (WebAuthenticatorIntermediateActivity.android.cs:127-134) is the correct and minimal observation point: if the activity is resumed after it already launched a Custom Tab or system browser and no callback claimed completion, the request is cancelled. Correctly, the AuthTab mode is excluded because ActivityResultLauncher always delivers a terminal result. The intent-extras + IsActive(requestId) identity check correctly prevents a stale activity from cancelling a newer request after process death. This is a real root-cause fix, not a symptom patch.
Layering. Placement is correct: shared lifecycle in src/Essentials/src/WebAuthenticator/*.shared.cs, platform mechanics in the platform files, route matching in the existing WebUtils. No new public API surface (WebAuthenticatorRequest/WebAuthenticatorRequestManager are internal; the removed IsAuthenticatingWithCustomTabs extension was internal), and PublicAPI.Unshipped.txt correctly untouched.
Concurrency review. The lock discipline is sound: CompletionClaimed is only read/written under locker, completion (including user ResponseDecoder invocation and BeforeCallbackCompletion) happens outside the lock, and TrySetResult/TrySetCanceled/TrySetException are all reserve-gated so double completion is impossible. RunContinuationsAsynchronously avoids inlining caller continuations onto the native callback thread. The iOS comment about DispatchQueue.DispatchAsync never running inline is accurate.
Memory/lifetime. The iOS session is cancelled and disposed on the main queue in finally, with GC.KeepAlive for the session and context provider; the cancellation Register handle is disposed in finally on all three platforms; the Android live-owner registry uses a WeakReference released in OnDestroy. No obvious leak introduced.
Concrete Findings
All findings are written to CustomAgentLogsTmp/PRState/37426/PRAgent/inline-findings.json with file:line anchors. Summary:
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | major | WebAuthenticator.android.cs:72 (also ios.tvos.cs:76, windows.cs:78) |
Begin(...) publishes the process-wide active request outside the try whose finally calls End(request). A throw from cancellationToken.Register (disposed CancellationTokenSource) leaks the slot permanently; every later AuthenticateAsync in the process then throws "Another WebAuthenticator operation is already in progress." |
| 2 | major | WebAuthenticatorRequestManager.shared.cs:37 |
Concurrency semantics change: previously a second call cancelled the pending one and proceeded (all three platforms did this explicitly). Now it throws InvalidOperationException. Double-tapped sign-in buttons and re-entrant navigation flows that worked before now throw out of the caller's handler. |
| 3 | major | WebAuthenticator.windows.cs:437 |
IsRegistryDeclared now additionally requires the registered shell\open\command executable to equal Environment.ProcessPath. Unpackaged apps registered against a launcher/installed exe path (versioned install dir, dev build run from a different output folder) now throw before the browser launches, even though Windows still activates them correctly. |
| 4 | moderate | WebUtils.shared.cs:145 |
Callback route matching tightened to exact, case-sensitive path equality. A provider redirect differing only by path casing, trailing slash, or percent-encoding is now silently dropped — reintroducing the hang class this PR fixes. Scheme/host are case-insensitive while the path is not. |
| 5 | moderate | WebAuthenticator.windows.cs:364 |
IsUriProtocolDeclared rewritten to match only the current <Application>; when the manifest has multiple <Application> elements and the app id cannot be resolved, a validly declared protocol is reported as undeclared and ValidateCallbackUrl throws. |
| 6 | moderate | WebAuthenticator.ios.tvos.cs:123 |
catch (Exception) discards the original exception with no inner exception (Android's equivalent path preserves it). This can convert a documented FeatureNotSupportedException into a generic InvalidOperationException and removes all field diagnostics. |
| 7 | moderate | WebAuthenticatorIntermediateActivity.android.cs:61 |
Null extras finishes the activity without completing or cancelling the active request — a residual indefinite-pending path of the same class as #32766. |
| 8 | moderate | WebAuthenticatorIntermediateActivity.android.cs:133 |
The resume-without-callback cancellation — the actual fix for #32766 — has no automated regression coverage. The added Android tests exercise helper predicates only; gate evidence is Windows device tests plus shared unit tests. |
Non-blocking observations (not filed inline)
ValidateModernSessionUrlsnow rejects a non-http/httpsstart URL on iOS/Mac Catalyst. This is almost certainly correct forASWebAuthenticationSession, but it is a new throw for input that previously reached the native API.- The iOS/tvOS file drops
SFAuthenticationSession,SFSafariViewController, andClearCookies. Given the current minimum iOS target this is appropriate, butPrefersEphemeralWebBrowserSessionno longer has any cookie-clearing fallback on tvOS; the doc comment was correspondingly relaxed to "Support varies by platform". WebAuthenticatorCallbackActivitynow always routes through the intermediate activity, even for the plain system-browser case that previously completed directly. This adds one transient activity launch on the callback path; visually it should be imperceptible, but it is a behavior change worth a manual smoke test.- Android uses
AuthTabIntent,CustomTabsClient.IsAuthTabSupported,IsEphemeralBrowsingSupported, andCustomTabsIntent.Builder.SetEphemeralBrowsingEnabledwith no change to the pinnedXamarin.AndroidX.Browser1.10.0.1ineng/AndroidX.targets, andComponentActivity/ActivityResultLauncherfrom AndroidX.Activity. Confirm these resolve on the pinned versions in CI on all Android TFMs (the review environment cannot build Android). WebAuthenticator.shared.csgives the built-in implementation first refusal on lifecycle callbacks before a customIWebAuthenticator. The behavior is documented in the class remarks and only consumes a matching route, so custom implementations still receive non-matching callbacks — acceptable, but it is a new ordering guarantee that is now part of the contract.
Blast Radius and Failure Probes
Blast radius. Every WebAuthenticator.AuthenticateAsync consumer on Android, iOS, Mac Catalyst, tvOS, and Windows. All three platform implementations were rewritten, the Android intermediate activity was replaced wholesale, the shared route-matching helper used by all platforms was tightened, and the callback activity's behavior changed. There is no opt-out or feature switch. This is the highest-risk shape of change for Essentials: a single shared lifecycle owner where every platform's failure and cancellation path is newly funnelled.
Failure probes to run before merge:
- Android, the reported repro (#32766): Custom Tab flow, press Back at the account chooser →
AuthenticateAsyncmust throwTaskCanceledExceptionpromptly, not hang. - Android, callback still wins the race: complete a real sign-in → result returned, and confirm the
OnResumecancel path did not fire first. - Android, system-browser fallback (no Custom Tabs provider installed): both the success and Back-cancel paths.
- Android, AuthTab HTTPS callback on a device with a supporting provider, plus the
ResultVerificationFailed/ResultVerificationTimedOutbranches. - Android, process death: background the app during authentication, let the OS kill it, return via callback → confirm no stale cancellation of a newer request and no hang.
- All platforms, double-invoke: call
AuthenticateAsynctwice in quick succession and confirm the newInvalidOperationExceptionis the intended contract (see finding #2). - All platforms, cancellation token: cancel mid-flight; then repeat authentication in the same process to prove the request slot was released (directly probes finding #1).
- Windows unpackaged: register the protocol via
ActivationRegistrationManager.RegisterForProtocolActivation, then run the app from a different executable path than the registered one (directly probes finding #3). - Windows packaged, multi-
<Application>manifest: confirm protocol declaration is still detected (probes finding #5). - Callback URL shape matrix: expected vs. actual differing only by path casing, trailing slash, or percent-encoding, and a custom scheme with a non-root path (probes finding #4).
- iOS/Mac Catalyst: custom-scheme callback success and user-cancel; HTTPS callback below 17.4 → confirm
FeatureNotSupportedException(not a genericInvalidOperationException) reaches the caller (probes finding #6).
Actionable Consolidated Feedback
A single follow-up patch could address the highest-value items; they are localized and mutually independent:
- Close the request-slot leak (findings #1). Move
Begin(...)inside thetry(or wrapBegin+Registerso any failure callsEnd) in all three platformAuthenticateAsyncmethods. Three small, mechanical edits; add a unit test that asserts a secondBeginsucceeds after a failed start. - Decide and document the concurrency contract (finding #2). Either restore cancel-previous semantics in
WebAuthenticatorRequestManager.Begin, or keep the throw and label it explicitly as a breaking change with migration guidance. One-line behavioral decision with large downstream impact. - Relax the two Windows guards to fail open where the previous code passed (findings #3 and #5). Treat a registered
URL Protocolkey as sufficient when the command path is a different-but-plausible executable, and fall back to the any-<Application>protocol match when the application id cannot be resolved. Add the two discriminating negative tests. - Soften or document the path-match tightening (finding #4), and add
WebUtils_Testscases for trailing slash, path casing, and percent-encoding so the intended semantics are pinned either way. - Preserve exception fidelity on iOS (finding #6) — pass the caught exception as the inner exception and rethrow
FeatureNotSupportedExceptionunwrapped, matching the Android pattern and the documented contract. - Add the Android regression test for the actual fix (finding #8), covering both the positive case (resume with no callback → cancelled) and the discriminating negative case (resume carrying a matching callback → not cancelled).
Items 1, 5, and 6 are low-risk and self-contained. Items 2 and 3 require a product decision from the author about intended contracts; if the author confirms the stricter behavior is deliberate, the patch reduces to documentation plus the missing negative tests.
🛠️ Try-Fix — Analysis & Comparison
PR #37426 — Try-Fix Candidates
Candidate 1 — claude-opus-5
Result: Blocked (no implementation applied, no tests run)
Approach (designed, NOT implemented)
Foreground-Resume Cancellation Watchdog, scoped to a single file:
src/Essentials/src/WebAuthenticator/WebAuthenticator.android.cs.
Keep the existing single tcsResponse field. For the lifetime of one AuthenticateAsync call,
subscribe to ActivityStateManager.Default.ActivityStateChanged. When the host application''s own
activity transitions back to Resumed after the browser/Custom Tab intent was started, and no
callback intent carrying Data has completed tcsResponse, cancel the pending TCS — posted to the
main thread, guarded by a "we actually left the app" flag set at browser-launch time, and by a short
post-resume grace window so a legitimate callback intent delivered on the same resume pass always
wins the race. The subscription is disposed in a finally alongside the existing
cancellationToken.Register scope. No new files, no static manager, no request identity, no change
to the Apple or Windows implementations.
How it differs mechanistically from PR #37426
PR #37426''s mechanism and root-cause hypothesis (ownership/identity): a process-wide
WebAuthenticatorRequestManager plus an identity-bearing WebAuthenticatorRequest, with a
request-owned Android WebAuthenticatorIntermediateActivity, and Android/Apple/Windows completion,
cancellation, and failure paths all routed through one exactly-once completion. The hypothesis is
that the pending task is ambient static state, so completion signals cannot be matched to the request
that produced them and the Back-press signal is lost. 22 files, +3015/-505.
This candidate''s root-cause hypothesis (missing observation point): the pending task is never
completed because the only component that observes dismissal is the Custom Tabs intermediate
activity — and that observation point does not exist at all on the system-browser fallback path
(AuthenticateAsync at merge-base ee4d06cde6 starts a bare Intent.ActionView browser intent with
no intermediate activity), and is not guaranteed to be resumed when the account-chooser task is torn
down by Back.
Cause-to-effect chain: Back at the Microsoft account chooser -> the browser task finishes -> the
app''s own activity is resumed -> ActivityStateChanged(Resumed) fires unconditionally, regardless of
which launch path was used and regardless of whether any intermediate activity survived -> the
watchdog observes a resume with no callback Data -> TrySetCanceled() -> AuthenticateAsync
returns TaskCanceledException instead of hanging forever.
The difference is not code location: PR #37426 makes the request identifiable so a broader set of
completion sites can complete it; this candidate makes the app returning to the foreground itself
the completion signal, so no additional completion site or identity is needed.
Files changed
None. No production file was modified. git status --short -- src/ is clean.
Captured diff
Explicitly empty. fix.diff and reviewer-findings.diff are both empty (0 bytes).
Test results
No tests were run. All three focused commands were intentionally NOT executed, because no
alternative implementation exists to validate. Running them against the unmodified worktree would
only re-measure the PR fix already present at HEAD — duplicating gate verification (explicitly out
of scope) and producing a misleading "Pass".
| # | Command | Status |
|---|---|---|
| 1 | dotnet test src/Essentials/test/UnitTests/Essentials.UnitTests.csproj --filter "FullyQualifiedName~WebAuthenticatorRequestManager_Tests" |
NOT RUN — nothing to validate |
| 2 | dotnet test src/Essentials/test/UnitTests/Essentials.UnitTests.csproj --filter "FullyQualifiedName~WebUtils_Tests" |
NOT RUN — nothing to validate |
| 3 | pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Essentials -Platform windows -TestFilter "Category=WebAuthenticator" |
NOT RUN — nothing to validate |
Per the try-fix skill, "Pass REQUIRES: the test command executed AND reported test success." No Pass
or Fail can be claimed. The alternative mechanism above is unimplemented and untested.
Pass / Fail / Blocked
Blocked.
Failure analysis — why Blocked (two independent, each sufficient, reasons)
1. Baseline could not be established — no .github/.baseline-state.json.
pwsh .github/scripts/EstablishBrokenBaseline.ps1 exited 1 with
ERROR: DIRTY WORKING DIRECTORY - Cannot establish baseline, listing 49 pre-existing modified/deleted
files under .github/scripts/, .github/skills/, and eng/scripts/ (plus ~22 untracked files).
These are unrelated harness/tooling changes that existed before this attempt; the caller and the skill
both require they be preserved exactly. The script''s own remediation (git checkout -- .) is
forbidden by skill Core Principle 6 (script-only restoration) and by the caller''s explicit
constraint, and would destroy unrelated work. Skill Step 2: "If the state file was not created ...
report Blocked immediately and proceed to Step 9 without modifying tracked files." Without
RevertedFiles there is no modification allow-list and, critically, no restore manifest — any edit
would be unrestorable via the only permitted cleanup command.
2. PR-added production files (NewFiles) — a pre-existing hard block.
git show --name-status HEAD confirms PR #37426 adds two production files:
src/Essentials/src/WebAuthenticator/WebAuthenticatorRequest.shared.cs(A)src/Essentials/src/WebAuthenticator/WebAuthenticatorRequestManager.shared.cs(A)
Had the baseline succeeded, NewFiles would have been non-empty, which Step 2 independently requires
be reported as Blocked before editing, because the restore script does not safely restore added
production files. The pre-flight document anticipated exactly this.
This is not merely procedural. The "broken" baseline would delete those two files while the
PR-added tests that reference WebAuthenticatorRequestManager remain, so
Essentials.UnitTests.csproj would not compile. Focused command (1) is therefore compile-coupled to
the very files a manager-free alternative would remove, and cannot act as a neutral oracle for this
candidate''s mechanism — the pre-flight already recorded this compile-coupling.
Inline self-review
0 findings (reviewer-findings.json = []). Performed inline per Step 6 against
.github/agents/maui-expert-reviewer.md; no maui-expert-reviewer sub-agent was spawned and no child
agents were used. With an empty change set, the Overarching Principles and the Always-Active
dimensions (Logic and Correctness, Regression Prevention, Complexity Reduction) are vacuously clean;
no file-routed dimensions applied. Step 7.5 drift check: reviewer-findings.diff is empty and matches
the final diff — zero drift, no refresh needed.
Restore confirmation
Ran exactly pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore. Output:
No baseline state found. Nothing to restore. -> Restored: False,
Message: No baseline state found.
This is the expected and accepted completion for the verified no-state path: Step 2 never created
baseline state, and zero attempt edits were made. Verified afterwards that git status --short -- src/
is empty — no attempt-created changes to any production file. All pre-existing unrelated dirty and
untracked .github/eng paths were left exactly as found; no git checkout/restore/reset/
clean/stash and no manual deletion was used at any point.
CustomAgentLogsTmp/PRState/37426/PRAgent/gate/content.md was neither created nor modified.
Artifacts
CustomAgentLogsTmp/PRState/37426/PRAgent/try-fix/attempt-1/ — baseline.log, approach.md,
result.txt (Blocked), fix.diff (empty), test-output.log, reviewer-findings.json ([]),
reviewer-findings.diff (empty), analysis.md. Artifact gate: PASSED.
Note for a future attempt
The designed mechanism is worth trying where the baseline can actually be established. Its appeal
over the PR mechanism is that it also covers the system-browser fallback path, which has no
intermediate activity and therefore no dismissal observation point in the pre-PR code. Its risks —
which testing would have targeted — are (a) a race where a legitimate callback intent arrives on the
same resume pass and must win, requiring a grace window; (b) spurious cancellation when the app is
resumed for an unrelated reason (multi-window, permission dialog) while auth is pending; and (c)
ActivityStateChanged being a static event, so the subscription must be deterministically
unsubscribed in a finally to avoid leaking across calls. These are real, and are plausibly why the
PR authors chose explicit request ownership instead. Environmental prerequisite: the unrelated
.github/eng tooling drift must be cleaned by the caller/harness before any try-fix attempt on
this PR can establish a baseline — that cleanup cannot be done from inside an attempt without
violating script-only restoration.
Candidate 2 — gpt-5.6-sol
Candidate 2 Try-Fix Attempt — Native Custom Tabs Hidden Event
Result: Blocked (no implementation applied; no tests run)
Approach (designed, NOT implemented)
Retain the pre-PR Android per-implementation TaskCompletionSource, but create its already service-bound CustomTabsSession with a CustomTabsCallback. When the browser service reports TAB_HIDDEN / NavigationEventTabHidden, atomically cancel the still-pending authentication task. This treats the browser protocol's explicit surface-dismissal notification as the cancellation signal. The system-browser fallback would remain unchanged because it has no Custom Tabs service signal.
Mechanistic difference
PR #37426 uses a process-wide identity-bearing request manager, centralizes completion across Android/Apple/Windows, and gives each Android request an intermediate activity whose return cancels the matching request. Candidate 1 proposed observing the host application's ActivityStateChanged(Resumed) event, with a left-app flag and grace window, then inferring cancellation when no callback arrived.
Candidate 2 uses neither mechanism. Its root-cause hypothesis is that the pre-PR Custom Tabs service session was already bound, but its native browser lifecycle callback was discarded. The proposed cause-to-effect chain is: Android Back closes the account chooser/custom tab -> the Custom Tabs service emits its native tab-hidden event -> that session callback cancels the pending TCS. There is no process-wide request identity, request-owned intermediate activity, host-activity resume observation, lifecycle watchdog, foreground-return heuristic, or grace window. This remains a design only; ordering against a successful deep-link callback and provider support would require empirical validation.
Files changed
None. Baseline establishment failed before creating a modification allow-list. No tracked production file was edited; pre-existing unrelated dirty/untracked files were preserved.
Captured diff
Explicitly empty. attempt-2/fix.diff and attempt-2/reviewer-findings.diff are 0 bytes because Candidate 2 applied no code change.
Focused validation
No tests were run because there was no safely applied Candidate 2 implementation. Running them against unchanged PR HEAD would repeat gate verification and validate the PR rather than this alternative.
| # | Command | Result |
|---|---|---|
| 1 | dotnet test src/Essentials/test/UnitTests/Essentials.UnitTests.csproj --filter "FullyQualifiedName~WebAuthenticatorRequestManager_Tests" |
NOT RUN — baseline blocked; no implementation |
| 2 | dotnet test src/Essentials/test/UnitTests/Essentials.UnitTests.csproj --filter "FullyQualifiedName~WebUtils_Tests" |
NOT RUN — baseline blocked; no implementation |
| 3 | pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Essentials -Platform windows -TestFilter "Category=WebAuthenticator" |
NOT RUN — baseline blocked; no implementation |
Pass requires actual focused tests, so no success is claimed. The mechanism is unimplemented and untested.
Pass / Fail / Blocked
Blocked. This final allowed alternative attempt is complete and does not need retry.
Failure analysis
pwsh .github/scripts/EstablishBrokenBaseline.ps1 exited 1 with ERROR: DIRTY WORKING DIRECTORY - Cannot establish baseline, listing extensive pre-existing unrelated modified/deleted .github and eng files. It created no .github/.baseline-state.json; therefore there was no RevertedFiles allow-list and no restoration manifest. The skill and caller explicitly require Blocked before editing when baseline state is absent. Cleaning or bypassing that state would destroy unrelated work or violate script-only restoration.
Independently, PR #37426 adds these production files:
src/Essentials/src/WebAuthenticator/WebAuthenticatorRequest.shared.cssrc/Essentials/src/WebAuthenticator/WebAuthenticatorRequestManager.shared.cs
A successful baseline would classify them as non-empty NewFiles, which is independently a mandatory pre-edit Blocked condition. The manager-focused test is also compile-coupled to those PR-added files, so it cannot validate a safely reverted manager-free candidate.
Inline maui-expert self-review
0 findings (reviewer-findings.json is []). The review was performed inline, without spawning a reviewer/child agent, against the Overarching Principles and the always-active Logic and Correctness, Regression Prevention, and Complexity Reduction dimensions, plus routed Input and Path Correctness. With an empty attempt diff there was no changed line to flag. The unimplemented design's callback-ordering/provider-support risks are explicitly documented and are not theoretical proof of correctness. The final empty diff matched reviewer-findings.diff; no refresh was needed.
Restore confirmation
Ran exactly:
pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore
Result: No baseline state found. Nothing to restore.; Restored: False; Message: No baseline state found. This is the expected accepted result for the verified no-state path: baseline state was never created and no attempt edit was made. No git checkout/restore/reset/clean/stash or manual deletion was used. The gate content file was not created or overwritten.
Artifacts
CustomAgentLogsTmp/PRState/37426/PRAgent/try-fix/attempt-2/ contains all mandatory files: baseline.log, approach.md, result.txt (Blocked), empty fix.diff, test-output.log, reviewer-findings.json ([]), empty reviewer-findings.diff, and analysis.md. Artifact gate passed.
🏁 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Winner: pr-plus-reviewer
The submitted architecture is the strongest fix for #32766: request identity and a request-owned Android intermediate activity cover Custom Tab and system-browser dismissal without the race-prone global foreground heuristics proposed by the alternatives. The reviewer-enhanced candidate is a strict improvement because it also preserves the underlying Apple session-start failure beneath the existing redacted exception, and every required focused validation completed successfully.
Candidate comparison
| Rank | Candidate | Implementation and evidence | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
Raw PR plus one exception-fidelity change; request-manager tests 27/27, URI tests 34/34, Windows device command exited 0 with 304 passed and 0 failed | Winner. Retains the PR's identity-safe lifecycle design while improving diagnosability without changing public behavior. Apple runtime coverage remains unverified on the Windows-only host. |
| 2 | pr |
Submitted fix; trusted Gate passed, including fail-without/pass-with-fix evidence | Correct root-cause architecture, but the Apple session-start catch discards the caught cause. The expert review also raised contract and Android coverage discussions that should be resolved before merge. |
| 3 | try-fix-1 |
Foreground-resume watchdog design only; blocked before editing, empty diff, no tests | Broader than a request-owned signal and susceptible to unrelated-resume and callback-ordering races. It has no executable evidence and therefore ranks below both passing candidates. |
| 4 | try-fix-2 |
Custom Tabs TAB_HIDDEN callback design only; blocked before editing, empty diff, no tests |
Provider-dependent and does not cover the system-browser fallback. It has no executable evidence and therefore ranks last. |
Expert review reconciliation
The expert reviewer produced the required initial evaluation and ten inline findings before refinement. The consolidated candidate addresses the actionable Apple exception-fidelity finding. Other findings were not changed in this one-pass candidate: concurrent-request rejection and ordinal callback-path matching are explicitly documented submitted-PR contracts; the Windows checks intentionally bind a declaration to the current application/executable; cancelling an Android request from an activity with no request id would violate identity safety; and direct Android lifecycle coverage cannot be executed on this Windows host. A target-runtime probe also did not reproduce the proposed disposed-token registration exception.
The remaining material uncertainty is Android end-to-end coverage of the actual Back/dismissal path and Apple compilation/runtime behavior. Under the execution contract, no additional repair or retest loop was attempted.
Required action
Apply pr-plus-reviewer/reviewer.patch to preserve the Apple inner exception, and resolve the remaining inline contract/coverage discussions. Because pr-plus-reviewer rather than the raw submitted PR wins, the required recommendation is REQUEST CHANGES.
📱 UI Tests — Essentials
Detected UI test categories: Essentials
[!WARNING]
No UI test results were produced for the detected categories. The PR build itself was
fine — the deep UI stage was skipped or interrupted on infrastructure (the
merge-for-testing step, emulator/simulator boot, or an Appium hang), not by this PR's code.
This is usually transient; the review re-runs on new commits (a maintainer can also re-run it).
🧭 Next Steps — reviewer patch required (pr-plus-reviewer)
The reviewer-enhanced candidate won, so the submitted PR still needs those changes.
Why: The submitted identity-safe lifecycle architecture is stronger and better validated than either blocked try-fix design. The reviewer-enhanced candidate wins because it preserves the Apple session-start cause as an inner exception while all required focused validations pass.
Apply PRAgent/pr-plus-reviewer/reviewer.patch from the CopilotLogs
artifact (or follow the report's Required submitted-PR change), push the update, and run
the review again.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions and verify if test failures are related?
|
Thanks @kubaflo for running the improved reviewer and flagging this follow-up. The separate follow-up commit
I also verified the remaining suggestions against the current implementation and target runtime. The proposed disposed-token registration leak was not reproducible on .NET 11; strict callback matching, Windows route ownership, and request-bound Android handling remain intentional contracts. Existing Android device tests exercise both browser-return cancellation and callback precedence. Validation for this pass (without device execution): focused unit tests 61/61, full Essentials unit tests 513/513, compile-only builds for .NET, .NET Standard, Android, Windows, iOS, Mac Catalyst, and Android DeviceTests, plus formatter and diff checks. The core build and DeviceTests CI were green; the non-green UI jobs were unrelated control/infrastructure failures, with no WebAuthenticator failure. All 16 follow-up threads have been answered and resolved. Thanks again for the detailed review. |
Description of Change
WebAuthenticator previously relied on platform-specific pending state and completion paths. This made lifecycle edge cases difficult to handle consistently: dismissing Android authentication UI could leave the awaiting task pending, concurrent requests could replace shared state, and callback, cancellation, and cleanup paths were not always bound to the same request identity.
This PR consolidates request ownership and completion while preserving the existing public API and WebAuthenticator's provider-agnostic role.
The change:
IWebAuthenticator;ASWebAuthenticationSessioncompletions terminal when their callback URI is missing or invalid;ASWebAuthenticationSessioncallback, cancellation, and cleanup handling on Apple platforms; HTTPS callbacks require iOS or Mac Catalyst 17.4+, the default port, and Associated Domains, while earlier OS versions must use a custom-scheme callback;AppInstanceroute for the process lifetime, avoiding the known unreliable unregister/re-register sequence while allowing a later WebAuthenticator route to replace it;AppInstancekey and documents the required cooperative activation routing;InnerException;NSErrordetails and caught session-start exceptions beneath redacted outer failures, while propagatingFeatureNotSupportedExceptionunchanged;A second valid built-in authentication throws
InvalidOperationExceptionwhile leaving the original request active.Applications should serialize authentication attempts, such as by disabling sign-in actions while a request is pending. This intentionally replaces the previous behavior where a new request canceled the first.
Callback matching is intentionally strict: scheme and host are case-insensitive, the effective port must match, and a non-root path uses ordinal equality. Query and fragment are ignored, and an expected root path does not constrain the callback path. This follows the redirect URI comparison requirements in RFC 6749 section 3.1.2.3 and the URI comparison rules in RFC 3986 section 6.2.2.1.
The persistent Windows route avoids
UnregisterKey()because Windows App SDK issue #4420 remains open. The replacement behavior is documented from the Windows App SDK 2.3.1 source and the A-to-B activation evidence from #36640.Issues Fixed
Fixes #32766
Related to #36640
Validation
git diff --check: passed.Thanks in advance!
@kubaflo