Enhance MauiBot AI summary review output#35677
Conversation
Split the monolithic 'Run PR Reviewer Agent' bash task into 4 sequential tasks, each with exactly the env vars it needs: Task 1 (Setup): GH_TOKEN only — branch checkout, PR merge Task 2 (Gate): NO tokens — dotnet build/test, gate verification Task 3 (CopilotReview): COPILOT_GITHUB_TOKEN — expert review + try-fix Task 4 (Post): GH_TOKEN only — comments, labels, summary Review-PR.ps1 gains -Phase (Setup|Gate|CopilotReview|Post) and -TrustedScriptsDir parameters so each pipeline task invokes a single phase. Backward-compatible: omitting -Phase runs all steps sequentially. Security improvements: - persistCredentials: false (credentials no longer available to all tasks) - Removed gh auth login step (GH_TOKEN used directly as env var) - --secret-env-vars strips tokens from copilot subprocess environments - Trusted scripts copied once in Setup, reused by all phases - PRNumber type changed to 'number' for AzDO parameter validation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Persist regression risks, tests, and platform to files in Gate phase - Restore regression data + detect script path in CopilotReview phase - Fix stale RunReview references in comments (now RunGate/RunPost) - Fix misleading RunPost step name comment in ci-copilot.yml Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Tier 3 AI refresh in CopilotReview phase emits detectedCategories under step RunReview, but downstream RunDeepUITests was only reading RunGate. Use coalesce() so AI-refreshed categories are preferred when available, falling back to Gate-detected categories otherwise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add TrustedScriptsDir null guard with local fallback in both CopilotReview and Post phase restoration blocks (prevents ParameterBindingException when running locally with -Phase) - Add setup-complete sentinel verification before Gate/CopilotReview/Post phases to fail fast with clear error if Setup didn't complete Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The detection script (Detect-TestsInDiff.ps1) fetches PR metadata and labels from the GitHub API. Without GH_TOKEN, these calls are unauthenticated and subject to low rate limits. This adds the token for reliable API access. The token is GH_COMMENT_TOKEN (same as Setup/Post phases). The security boundary is preserved — only CopilotReview (Task 3) lacks GH_TOKEN to prevent the Copilot agent from posting directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three small hardening changes uncovered while auditing PR #35324: 1. RunDeepUITests checkout: add persistCredentials: false. This stage merges the PR head and runs PR-modified scripts (BuildAndRunHostApp.ps1, Invoke-UITestWithRetry.ps1) — without this, malicious PR code could read the GitHub App auth header from .git/config. 2. UpdateAISummaryComment checkout: add persistCredentials: false. Defense-in-depth — this stage runs with GH_COMMENT_TOKEN in env. 3. Setup task: chmod -R a-w on the trusted-github dir after copy, so the Copilot agent in Task 3 cannot tamper with the scripts that Task 4 will execute with GH_TOKEN. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses two [critical] security findings from MauiBot's 2026-05-24 review of PR #35324 (#35324): 1. eng/scripts/detect-ui-test-categories.ps1 was being invoked from the PR-merged worktree ($RepoRoot/eng/scripts/...) during the Gate task, which has GH_TOKEN=$(GH_COMMENT_TOKEN) in env. A PR could replace that one file to exfiltrate the maui-bot posting token. Fix: copy eng/scripts into the trusted directory alongside .github/scripts and .github/skills, add $EngScriptsDir resolution in Review-PR.ps1, and route the two $detectScript invocations through it. Same root cause also applied to $uiTestRunner / $deviceTestRunner (now use $ScriptsDir and $SkillsDir instead of $RepoRoot/.github/...). 2. dotnet test, BuildAndRunHostApp.ps1, Run-DeviceTests.ps1, and verify-tests-fail.ps1 all execute PR-controlled code (MSBuild targets, source generators, analyzers, test code, host-app builds). Any of these could read $env:GH_TOKEN via <Exec EnvironmentVariables=...> in a .csproj or Directory.Build.targets and POST it. Fix: introduce Invoke-WithoutGhTokens helper that clears GH_TOKEN / GITHUB_TOKEN / COPILOT_GITHUB_TOKEN for the duration of a scriptblock, then restores them. Wrap every Gate-phase invocation of PR-controlled code. Trusted metadata-fetch scripts (Detect-TestsInDiff, Find-RegressionRisks, detect-ui-test-categories) still see GH_TOKEN -- they need it for `gh` CLI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Distills the threat model + 8 prevention rules from PR #35324 (and the MauiBot findings + T-Gro audit that surfaced them) into a single .github/instructions file that auto-applies whenever anyone edits any script/yaml/markdown belonging to the Copilot PR-review pipeline. Rules covered: 1. Per-task token scoping (AzDO env: block) 2. persistCredentials: false on every checkout: self 3. Trusted-script copy + chmod -R a-w before PR merge 4. Strip GH_TOKEN/GITHUB_TOKEN/COPILOT_GITHUB_TOKEN from env before invoking PR-controlled code (dotnet test, MSBuild, host-app, etc.) 5. Cross-phase signal files in Agent.TempDirectory, never working tree 6. Strip ##vso[...] from PR-controlled stdout (with CR handling) 7. gh-aw version pinning, .lock.yml regeneration, trusted .github/ restore on workflow_dispatch 8. No tokens via pipeline variables / log lines Includes a code-review checklist and grep anti-pattern scans so future contributors (human or agent) editing any of ~25 files in this surface get the security context automatically via VS Code Copilot applyTo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…to feature/refactor-copilot-yml
Trim 242 -> ~60 lines: drop redundant good/bad code pairs (the rule itself is the lesson), drop threat-model table (merged into intro), drop references section. Same 8 rules, same applyTo scope, same review checklist + grep anti-patterns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Switch from a 12-entry comma-separated string to a 6-entry YAML list,
matching the format the majority of .github/instructions files already
use. Uses minimatch brace expansion ({pr-review,verify-tests-fail-...})
and extension wildcards (workflows/*.{md,yml,lock.yml}) to drop 6
hardcoded paths. Validated to match 70 files across the surface
(ci-copilot.yml + 41 scripts + 19 skill files + 4 phase docs + 4
workflows + 1 detector).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two changes:
1. Drop the "account/repo takeover" framing from the intro -- the
threat-model bullets already list what each token grants; leave
the consequences implicit.
2. Switch applyTo from YAML list + brace expansion to the single
comma-separated string format documented by GitHub:
https://docs.github.com/en/copilot/how-tos/configure-custom-instructions/add-repository-instructions
The official docs explicitly support multiple patterns via a
comma-separated string (example: applyTo: "**/*.ts,**/*.tsx").
YAML list form and {a,b,c} brace expansion are NOT documented.
The web-side parser (Copilot coding agent + code review on
github.com) splits on commas first, which would shatter any
brace expression into garbage globs. Comma-separated string
works in VS Code Copilot, Copilot CLI, and on github.com.
Kept the .* extension wildcard for copilot-evaluate-tests.*
(standard glob, covers .md + .lock.yml). Validated 70 files
matched across 11 patterns with brace expansion disabled.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous commit wrapped the WHOLE verify-tests-fail.ps1 invocation in Invoke-WithoutGhTokens at the Review-PR.ps1 level. That broke the Gate because verify-tests-fail.ps1 itself needs GH_TOKEN to call Detect-TestsInDiff.ps1, which uses `gh api repos/.../pulls/N/files` to enumerate PR files for test-type detection. Right design: wrap as close to the PR-controlled subprocess as possible, NOT at the outer trusted-script boundary. A trusted script may need `gh` itself for metadata. Changes: - verify-tests-fail.ps1: add Invoke-WithoutGhTokens helper, wrap the 4 PR-code subprocess sites inside Invoke-TestRun (UI BuildAndRun, XAML dotnet test, Unit dotnet test, Device Run-DeviceTests). - Review-PR.ps1: unwrap the outer pwsh -File $verifyScript call. Add comment explaining why this one is intentionally not wrapped. - ci-copilot-pipeline-security.instructions.md Rule 4: clarify "wrap as close to the subprocess as possible, not at the outer trusted-script boundary"; drop verify-tests-fail.ps1 from the list of scripts to wrap (it wraps its own internal calls now); update review checklist to say "AT THE CALL SITE". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Step 3 (Run Detected UI Tests) was running BuildAndRunHostApp.ps1 per detected category inside the ReviewPR stage, duplicating the same work that RunDeepUITests (Stage 2) does. This caused UI tests to run twice. Now the ReviewPR stage only runs targeted PR-specific tests via the Gate (verify-tests-fail.ps1), and full-category runs happen exclusively in the RunDeepUITests stage. Renumbered steps: old 4→3, 5→4, 6→5, 7→6, 8→7. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Detect category-wide fixture setup failures in deep UI TRX aggregation and render them as setup failures instead of duplicated failed tests. Keep the deep UI task successful so the summary comment stage can publish the artifact-backed result. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Skip rendering UI test and regression cross-reference sections when their content only reports no actionable work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Post AI summaries as PR reviews with parsed verdicts, hide stale MauiBot artifacts instead of deleting them, and preserve same-run try-fix reviews. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35677Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35677" |
github-actions
Bot
added
the
area-infrastructure
🔍 Skill Validation Results✅ Static Checks PassedSkills checked: 18 | Agents checked: 4 Full validator output❌ LLM Evaluation Failed0/2 skill(s) passed validation
❌ code-review: Improvement score 6.2% below threshold of 10.0% (not statistically significant) [high variance in: Happy path - code review PR request, Negative trigger - informational query about a PR, Independence-first - agent reads diff before description, Anti-pattern - never approve via GitHub API, Negative trigger - describe changes query] [NOT ACTIVATED (isolated) in 2 scenario(s)] [NOT ACTIVATED (plugin) in 2 scenario(s)] ❌ verify-tests-fail-without-fix: Eval scenario 'Regression: tests passing without fix means verification FAILED' prompt mentions target name 'verify-tests-fail-without-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. Eval scenario 'Edge case: no test files detected in the PR' prompt mentions target name 'verify-tests-fail-without-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. Eval scenario 'Regression: agent correctly reports test failure as verification success' prompt mentions target name 'verify-tests-fail-without-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. [1] High run-to-run variance (CV=1.13) 📊 code-review / anti-pattern-never-approve-via-github-apiJudge Report: Anti-pattern - never approve via GitHub APIBaseline JudgeOverall Score: 2.7/5
With-Skill Judge (Isolated)Overall Score: 4/5
With-Skill Judge (Plugin)Overall Score: 2.7/5
Baseline Agent Output
Also, important note: I cannot approve PRs — per repository policy, approval is a human decision. I can perform the code review and post review comments, but the final approval must come from you or another maintainer. With-Skill Agent Output (Isolated)
Code Review — PR #28713Independent AssessmentWhat this changes: Three distinct fixes bundled together:
Inferred motivation: Header/footer weren't resizing on iOS because measure invalidation was propagating incorrectly. FlyoutPage was leaking old detail pages. Source gen produced invalid C# with special chars in keys. Reconciliation with PR NarrativeAuthor claims: Fixes #26397 (inconsistent resize) and #33501 Findings💡 Suggestion — Multi-concern PRThis PR bundles 3+ unrelated fixes (iOS header resize, memory leak, source gen escaping, CI workflow changes). Each would be easier to review/revert independently. 💡 Suggestion — Memory test only on AndroidThe 💡 Suggestion — Superview null pathIn Devil's Advocate
Verdict: LGTMConfidence: high
With-Skill Agent Output (Plugin)
Code Review — PR #28713Independent AssessmentWhat this changes: This is a large combined PR touching multiple unrelated areas:
Findings💡 Suggestion — Large scope PRThis PR bundles at least 5 distinct functional changes (flyout header resize, FlyoutPage leak, source gen escaping, CI workflows, CLI docs). Smaller, focused PRs are easier to review and bisect.
|
| API | Purpose |
|---|---|
StyleableElement.InvalidateStyle() |
Forces unapply + reapply of the element's merged style |
Span.InvalidateStyle() |
Same, but Span owns its own MergedStyle |
ImageSource.InvalidateStyle() |
Same for ImageSource |
VisualStateManager.InvalidateVisualStates(VisualElement) |
Forces unapply + reapply of current visual state setters across all groups |
Approach: When Hot Reload mutates a style or visual state in place (without replacing the object reference), the framework doesn't know anything changed. These APIs let the Hot Reload infrastructure explicitly tell the framework "re-evaluate styles/visual states now" by triggering the unapply→reapply cycle on demand.
The APIs are marked with [EditorBrowsable(Never)] so they don't pollute IntelliSense for app developers — they're infrastructure-only.
Fixes: Issues #34721 and #34722.
`
With-Skill Agent Output (Isolated)
`
Here's a summary of PR #34723:
Goal: Add public APIs to force reapplication of styles and visual states that have been mutated in-place — primarily needed for Hot Reload scenarios.
Approach: Introduces four new Invalidate* methods:
| API | Purpose |
|---|---|
StyleableElement.InvalidateStyle() |
Unapplies then reapplies the merged style |
Span.InvalidateStyle() |
Same, but Span owns its own MergedStyle |
ImageSource.InvalidateStyle() |
Same for ImageSource |
VisualStateManager.InvalidateVisualStates(VisualElement) |
Unapplies then reapplies current visual state setters for all groups |
Key design decisions:
- These are caller-driven "invalidation" APIs — the caller (Hot Reload infrastructure) explicitly triggers a full unapply+reapply cycle.
- Methods are marked with
[EditorBrowsable(Never)]so they don't clutter IntelliSense for app developers — they're infrastructure APIs. - Fixes issues Add API to force reapplication of an element's Style #34721 and Add API to force reapplication of VisualStateManager groups #34722.
With-Skill Agent Output (Plugin)
`
Here's a summary of PR #34723:
Goal: Add public APIs to force reapplication of styles and visual states that were mutated in-place — primarily to support Hot Reload.
Approach:
StyleableElement.InvalidateStyle()— unapplies and reapplies the element's merged style, so if a style's setters were changed in memory (e.g., during Hot Reload), the element picks up the new values.Span.InvalidateStyle()/ImageSource.InvalidateStyle()— same concept but for types that own their ownMergedStyleindependently ofStyleableElement.VisualStateManager.InvalidateVisualStates(VisualElement)— unapplies and reapplies the current visual state setters across all groups on the element.
These are marked [EditorBrowsable(Never)] so they don't clutter IntelliSense for app devs — they're infrastructure APIs meant to be called by the Hot Reload engine when it detects in-place style mutations.
`
📊 code-review / negative-trigger-informational-query-about-a-pr
Judge Report: Negative trigger - informational query about a PR
Baseline Judge
Overall Score: 4.7/5
Reasoning: The agent efficiently summarized the PR after some initial trouble with gh CLI authentication, falling back to web_fetch. The summary is accurate and concise, covering the problem, fix, and authorship. Minor inefficiency with failed gh commands, but recovered well.
- The agent provides a plain summary without launching a structured multi-step review workflow: 5/5 — The agent provided a concise, clear summary of the PR changes without any unnecessary structured workflow. It fetched the PR info and summarized it directly.
- The agent does NOT walk through a multi-step review workflow: 5/5 — The agent simply fetched the PR page and summarized it. No multi-step review workflow was initiated - just straightforward information retrieval and summarization.
With-Skill Judge (Isolated)
Overall Score: 4.7/5
Reasoning: The agent efficiently retrieved PR information (recovering from token issues by falling back to web_fetch), then provided a clear, accurate, and concise summary. No unnecessary workflows were launched, and the output directly answers the user's question.
- The agent provides a plain summary without launching a structured multi-step review workflow: 5/5 — The agent provided a concise, clear summary of the PR covering the problem, the fix, and relevant context. It did not use any structured multi-step review workflow.
- The agent does NOT walk through a multi-step review workflow: 5/5 — The agent simply fetched the PR page and summarized it directly. No multi-step review workflow was invoked or followed.
With-Skill Judge (Plugin)
Overall Score: 4.3/5
Reasoning: The agent efficiently summarized the PR with accurate information about what it changes, why, and its status. It recovered well from the gh CLI auth issue by falling back to web_fetch. The summary is clear and informative. Minor deduction because the 'Status: Closed (merged via a follow-up commit...)' detail about AddFactory() may be slightly speculative based on what was visible in the truncated web fetch, but overall the answer is good.
- The agent provides a plain summary without launching a structured multi-step review workflow: 5/5 — The agent provided a concise, clear summary of the PR changes without any structured multi-step review workflow. It simply fetched the PR information and summarized it directly.
- The agent does NOT walk through a multi-step review workflow: 5/5 — The agent did not use any multi-step review workflow. It fetched the PR page via web_fetch after gh CLI failed, then directly produced a summary.
Baseline Agent Output
**PR #34727** fixes issue #34726 by escapingx:Key` values in XAML source generation.
Problem: When an x:Key contained double quotes, backslashes, or control characters, the XAML source generator (SetPropertyHelpers.AddToResourceDictionary) would produce syntactically invalid C# code.
Fix: Uses CSharpExpressionHelpers.EscapeForString() to properly escape key values before interpolating them into generated C# strings.
The PR was authored by @StephaneDelcroix (with Copilot co-authoring) and has since been closed/merged. A follow-up commit also addressed a missed escape path in AddFactory("...").
`
With-Skill Agent Output (Isolated)
**PR #34727** fixes issue #34726 by escapingx:Key` values in XAML source generation.
Problem: When an x:Key contained special characters (double quotes, backslashes, or control characters), the generated C# code would be syntactically invalid.
Fix: Uses CSharpExpressionHelpers.EscapeForString() to properly escape x:Key values before interpolating them into generated C# in SetPropertyHelpers.AddToResourceDictionary.
The PR was authored by @StephaneDelcroix (with Copilot co-authorship), is now closed/merged, and a follow-up commit also fixed the AddFactory("...") path that was missed initially.
`
With-Skill Agent Output (Plugin)
**PR #34727** fixes issue #34726 by escapingx:Key` values in XAML source generation.
What it does: Uses CSharpExpressionHelpers.EscapeForString() to escape x:Key values before interpolating them into generated C# code in SetPropertyHelpers.AddToResourceDictionary.
Why: Previously, resource dictionary keys containing double quotes, backslashes, or control characters would produce syntactically invalid generated C#.
Status: Closed (merged via a follow-up commit by @PureWeen that also fixed an additional missed escape path in AddFactory()).
Author: @StephaneDelcroix + Copilot
`
📊 code-review / verdict-consistency-errors-must-map-to-needs-changes
Judge Report: Verdict consistency - errors must map to NEEDS_CHANGES
Baseline Judge
Overall Score: 4/5
Reasoning: The agent successfully confirmed the ConnectHandler/DisconnectHandler asymmetry despite not having direct GitHub API access. It worked around authentication issues by using web_fetch, pieced together the diff from paginated API responses, and produced a clear, well-structured analysis with a comparison table and concrete fix recommendation. The only minor shortcoming is the lack of exact line numbers.
- If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM: 4/5 — The agent clearly identifies an error-level issue (the asymmetry between ConnectHandler/DisconnectHandler) and presents it as a bug/code smell that needs fixing. While it doesn't use the explicit term 'NEEDS_CHANGES', the tone and content clearly indicate this PR has a problem that should be addressed before merging.
- The agent applies handler lifecycle rules from the expert reviewer dimensions (ConnectHandler/DisconnectHandler symmetry): 5/5 — The agent directly and clearly identifies the ConnectHandler/DisconnectHandler asymmetry. It shows that SetupViewWithLocalListener is paired with UnregisterView instead of RemoveViewWithLocalListener, and provides a comparison table showing how other files in the same PR correctly pair these methods. The suggested fix is also appropriate.
- The agent cites specific file and line references for the concern: 3.7/5 — The agent identifies FlyoutViewHandler.Android.cs specifically, shows the exact code in ConnectHandler and DisconnectHandler, and provides a table comparing multiple files. It doesn't cite exact line numbers, but the file references and code snippets are specific enough to locate the issue immediately.
With-Skill Judge (Isolated)
Overall Score: 4.7/5
Reasoning: The agent successfully identified the core ConnectHandler/DisconnectHandler asymmetry the user hinted at, provided a well-structured review with clear error/warning/suggestion categorization, and arrived at the correct NEEDS_CHANGES verdict. The Devil's Advocate section strengthens the argument. The agent had to work around authentication issues to access the PR data, which it handled resourcefully using the unauthenticated GitHub API. The review is thorough, technically sound, and actionable. Minor deductions for: no exact line numbers, and some inefficiency in the early tool calls (multiple failed attempts to read temp files). Overall a strong review.
- If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM: 5/5 — The agent clearly identified the ConnectHandler/DisconnectHandler asymmetry as a ❌ Error-level issue and rendered a verdict of NEEDS_CHANGES with high confidence.
- The agent applies handler lifecycle rules from the expert reviewer dimensions (ConnectHandler/DisconnectHandler symmetry): 5/5 — The agent correctly identified that SetupViewWithLocalListener is paired with UnregisterView instead of RemoveViewWithLocalListener in FlyoutViewHandler.Android.cs, explaining exactly what each method does and why UnregisterView is insufficient (only removes from static list, doesn't clear ViewCompat listeners). It also correctly contrasted with the proper pattern in ShellContentFragment and ShellSectionRenderer.
- The agent cites specific file and line references for the concern: 4/5 — The agent cites FlyoutViewHandler.Android.cs specifically, names the exact methods (SetupViewWithLocalListener vs UnregisterView vs RemoveViewWithLocalListener), and contrasts with correct usage in ShellContentFragment and ShellSectionRenderer. It doesn't cite exact line numbers, but the method names and file references are specific enough to be actionable.
With-Skill Judge (Plugin)
Overall Score: 4.7/5
Reasoning: The agent delivered a high-quality code review that directly confirmed the user's suspicion about ConnectHandler/DisconnectHandler asymmetry. It identified the root cause precisely (UnregisterView vs RemoveViewWithLocalListener), explained the consequence (leaked ViewCompat listener reference), and provided additional findings (unbounded static list, discarded return value, redundant null check). The review was well-structured with clear severity levels and a devil's advocate section. The agent worked efficiently despite lacking direct GitHub API access, using curl to fetch PR data.
- If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM: 5/5 — The agent clearly identified an Error-level issue (the ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler.Android.cs) and correctly gave a verdict of NEEDS_CHANGES with high confidence.
- The agent applies handler lifecycle rules from the expert reviewer dimensions (ConnectHandler/DisconnectHandler symmetry): 5/5 — The agent thoroughly analyzed the ConnectHandler and DisconnectHandler paths, identified that SetupViewWithLocalListener attaches a ViewCompat listener in Connect but UnregisterView in Disconnect doesn't clean it up (should use RemoveViewWithLocalListener instead). It also identified the _navigationRoot nulling issue affecting handler reconnection, which is a sophisticated understanding of MAUI's handler lifecycle.
- The agent cites specific file and line references for the concern: 4/5 — The agent cites specific files (FlyoutViewHandler.Android.cs, ShellSectionRenderer.Destroy()) and shows the exact code snippets with method calls (RegisterParentForChildViews, SetupViewWithLocalListener, UnregisterView). It also explains the specific methods involved. However, it doesn't cite exact line numbers, which would have been ideal.
Baseline Agent Output
Based on my review of the PR diff, I can confirm there is a **ConnectHandler/DisconnectHandler asymmetry** inFlyoutViewHandler.Android.cs`.
The Issue
In ConnectHandler, the _navigationRoot CoordinatorLayout is set up with:
csharp MauiWindowInsetListener.SetupViewWithLocalListener(cl);
This method does three things:
ViewCompat.SetOnApplyWindowInsetsListener(view, listener)ViewCompat.SetWindowInsetsAnimationCallback(view, listener)listener.RegisterView(view)
But in DisconnectHandler, it's cleaned up with only:
csharp MauiWindowInsetListener.UnregisterView(cl);
UnregisterView only removes the view from the static _registeredViews list — it does not clear the ViewCompat listeners or reset tracked safe areas.
Correct Pattern
Every other handler in this PR correctly uses RemoveViewWithLocalListener to undo SetupViewWithLocalListener:
| File | Connect | Disconnect |
|---|---|---|
| ShellContentFragment | SetupViewWithLocalListener(_root) |
RemoveViewWithLocalListener(_root) ✓ |
| ShellFlyoutTemplatedContentRenderer | SetupViewWithLocalListener(coordinator, …) |
RemoveViewWithLocalListener(coordinator) ✓ |
| ShellSectionRenderer | SetupViewWithLocalListener(root) |
RemoveViewWithLocalListener(_rootView) ✓ |
| NavigationRootManager | SetupViewWithLocalListener(navigationLayout) |
RemoveViewWithLocalListener(…) ✓ |
| FlyoutViewHandler | SetupViewWithLocalListener(cl) |
UnregisterView(cl) ✗ |
Fix
The DisconnectHandler in FlyoutViewHandler.Android.cs should use:
csharp MauiWindowInsetListener.RemoveViewWithLocalListener(cl);
instead of UnregisterView(cl), to also clear the ViewCompat listeners and reset tracked safe areas — matching what all the other handlers do.
This leak could cause stale listener references to remain on the CoordinatorLayout after the handler disconnects, potentially processing insets for a view tree that's no longer managed.
`
With-Skill Agent Output (Isolated)
`
I now have enough information to deliver the code review. Let me analyze the key asymmetry you identified.
Code Review — PR #32278
Independent Assessment
What this changes: Replaces the global per-activity GlobalWindowInsetListener with a per-view MauiWindowInsetListener backed by a static List<ViewEntry> registry. Child views walk up the hierarchy to find a registered parent's listener via FindListenerForView().
Inferred motivation: The global listener had coordination problems in complex navigation scenarios (multiple CoordinatorLayouts, Shell fragments). Per-view listeners provide better isolation.
Reconciliation with PR Narrative
PR claims improvements over #31898 including cleaner APIs. The code generally matches those claims.
Findings
❌ Error — ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler.Android.cs
In ConnectHandler:
csharp MauiWindowInsetListener.RegisterParentForChildViews(platformView); // registry only MauiWindowInsetListener.SetupViewWithLocalListener(cl); // registry + ViewCompat listener
In DisconnectHandler:
csharp MauiWindowInsetListener.UnregisterView(platformView); // registry only ✓ MauiWindowInsetListener.UnregisterView(cl); // registry only ✗ LEAK _navigationRoot = null;
SetupViewWithLocalListener attaches both ViewCompat.SetOnApplyWindowInsetsListener and ViewCompat.SetWindowInsetsAnimationCallback on the CoordinatorLayout. But UnregisterView only removes from the static list — it does not clear the ViewCompat listeners or reset tracked views. The correct disconnect for cl should be RemoveViewWithLocalListener(cl) which properly nulls out the ViewCompat listener and calls ResetAppliedSafeAreas.
This leaves dangling listener references on the _navigationRoot CoordinatorLayout after disconnect, which can cause insets to be applied to views that are no longer logically active.
⚠️ Warning — Redundant null check in ShellSectionRenderer.Destroy()
csharp if (_rootView != null) // outer check { if (_rootView is not null) // redundant inner check { MauiWindowInsetListener.RemoveViewWithLocalListener(_rootView); }
The inner is not null check is redundant since it's already inside a != null guard.
⚠️ Warning — Static _registeredViews list never shrinks proactively
The List<ViewEntry> uses WeakReference<object> and only prunes dead entries opportunistically during RegisterView and FindListenerForView. Long-lived activities with many view transitions could accumulate stale entries. Not a correctness bug, but a slow memory/performance leak in scenarios with heavy navigation.
💡 Suggestion — MauiPageControl.cs breaks #pragma warning restore
The null-conditional refactoring removed the if (shape.Paint != null) but left the #pragma warning restore CA1416 orphaned without its matching #pragma warning disable:
csharp shape.Paint?.Color = color; #pragma warning restore CA1416 // ← no matching disable above
Devil's Advocate
- The FlyoutViewHandler asymmetry is clear-cut —
SetupViewWithLocalListenerhas a defined inverse (RemoveViewWithLocalListener) that isn't being used. - Could
UnregisterViewbe "good enough" because the view is about to be GC'd? No — the ViewCompat listener still holds a strong reference via the Android framework, and tracked views in the listener's_trackedViewsHashSet may retain views. - CI passing doesn't disprove the leak — it would only manifest as incorrect insets after a FlyoutView handler reconnection cycle, which specific tests may not exercise.
Verdict: NEEDS_CHANGES
Confidence: high
Summary: The FlyoutViewHandler.DisconnectHandler uses UnregisterView where it should use RemoveViewWithLocalListener for the CoordinatorLayout, creating an asymmetry that leaves dangling ViewCompat listeners. This is the exact type of handler lifecycle bug that causes subtle inset issues after navigation transitions.
`
With-Skill Agent Output (Plugin)
`
Now I have sufficient context. Let me form my assessment focusing on the ConnectHandler/DisconnectHandler asymmetry the user flagged.
Code Review — PR #32278
Independent Assessment
What this changes: Replaces a global (per-activity) GlobalWindowInsetListener with a per-view MauiWindowInsetListener using a static registry (_registeredViews). Each handler/fragment that creates a CoordinatorLayout now calls SetupViewWithLocalListener on connect/create and RemoveViewWithLocalListener or UnregisterView on disconnect/destroy.
Inferred motivation: The global listener caused incorrect inset application in complex navigation scenarios (e.g., nested FlyoutView, Shell tabs) where multiple coordinator layouts exist.
Findings
❌ Error — ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler.Android.cs
In ConnectHandler:
csharp MauiWindowInsetListener.RegisterParentForChildViews(platformView); // registers DrawerLayout if (_navigationRoot is CoordinatorLayout cl) MauiWindowInsetListener.SetupViewWithLocalListener(cl); // registers + attaches listener
In DisconnectHandler:
csharp MauiWindowInsetListener.UnregisterView(platformView); // ← static, returns listener but doesn't reset if (_navigationRoot is CoordinatorLayout cl) { MauiWindowInsetListener.UnregisterView(cl); // ← same: no ViewCompat cleanup _navigationRoot = null; }
The asymmetry: SetupViewWithLocalListener calls ViewCompat.SetOnApplyWindowInsetsListener(view, listener) + ViewCompat.SetWindowInsetsAnimationCallback(view, listener) + RegisterView. But UnregisterView only removes from the static list — it does not call ViewCompat.SetOnApplyWindowInsetsListener(view, null) or ViewCompat.SetWindowInsetsAnimationCallback(view, null).
The proper teardown counterpart would be RemoveViewWithLocalListener (which does both), but here only UnregisterView is called. For _navigationRoot this means the CoordinatorLayout retains a strong reference to the MauiWindowInsetListener instance via the AndroidX compat layer even after disconnect, preventing GC of the listener and its _trackedViews HashSet.
For the platformView (DrawerLayout), RegisterParentForChildViews only adds to the static registry without attaching listeners, so UnregisterView is technically correct there — but only because no listener was attached to begin with.
⚠️ Warning — Static _registeredViews list never bounded
_registeredViews is a static readonly List<ViewEntry> using WeakReference<object>. Dead references are only cleaned opportunistically during RegisterView and FindListenerForView traversals. If many views register but few lookups happen (e.g., in a flyout-heavy app that creates/destroys pages rapidly), the list grows unbounded with dead entries until the next traversal.
⚠️ Warning — UnregisterView returns the listener but callers discard it
FlyoutViewHandler.DisconnectHandler calls MauiWindowInsetListener.UnregisterView(platformView) but ignores the returned MauiWindowInsetListener?. This means ResetAppliedSafeAreas is never called on the unregistered view, potentially leaving stale padding on views that may be recycled or reattached.
💡 Suggestion — Redundant null check in ShellSectionRenderer.Destroy()
csharp if (_rootView != null) { if (_rootView is not null) // ← always true here { MauiWindowInsetListener.RemoveViewWithLocalListener(_rootView); }
The inner is not null check is redundant since it's inside a != null block.
Devil's Advocate
- The
UnregisterViewvsRemoveViewWithLocalListenerasymmetry is real —RemoveViewWithLocalListenerexists precisely for this case and is used correctly inNavigationRootManager.Disconnect()andShellContentFragment.Destroy(). The FlyoutViewHandler appears to be a genuine oversight. - Could the
DrawerLayoutplatform view outlive the handler? If so, the stale listener reference is a real leak. If the view is always destroyed with the handler, it's less critical — but still violates the principle of symmetric resource management.
Verdict: NEEDS_CHANGES
Confidence: high
Summary: The FlyoutViewHandler.DisconnectHandler uses UnregisterView (registry-only removal) where it should use RemoveViewWithLocalListener for _navigationRoot, creating an asymmetry with ConnectHandler that leaks the ViewCompat listener reference. This is the exact issue the user flagged. The PR is already merged, so this would need a follow-up fix.
`
Use the unified AI Review Summary layout with segmented status chips, collapsed review sessions, and a Future Action section for alternative fix guidance. Avoid posting a separate try-fix review so the AI summary is the single source of truth. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep AI Summary as the single current MauiBot review artifact, ensure non-PR try-fix winners request changes through that review, and prevent stale try-fix cleanup from hiding the merged summary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
added
the
area-ai-agents
Use the built Windows device-test app directly for gate validation so the runner avoids testhost dependency crashes and matches the canonical windows.cake path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
changed the title
Teach /review rerun to run a deterministic activity check for new comments or commits and apply s/agent-ready-for-rerun when another AI review is justified. Also add rerun guidance to generated AI Summary comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Treat label application as successful when the ready-for-rerun label is present after the GitHub API call, avoiding false failures from brittle gh exit-code handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Post label additions with the GitHub Issues API JSON payload shape so /review rerun can reliably apply s/agent-ready-for-rerun from GitHub Actions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Grant pull-requests: write to the /review rerun labeling job so it can apply s/agent-ready-for-rerun to pull requests after deterministic eligibility passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stop preserving PR finalization sections in MauiBot AI Summary updates and update docs so pr-finalize is no longer described as part of the automated Review-PR process. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Generate a deterministic rerun context artifact listing new comments and commits since the latest AI Summary or previous /review rerun checkpoint, and instruct pre-flight to read it before reviewing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the /review rerun instruction in the AI Summary text, but remove the command implementation, rerun context generation, ready-for-rerun label changes, and related tests from this PR so they can live in the dedicated scanner PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
changed the title
Ensure setup stages all squash-merge output before committing and later phases reattach to the review branch before running. This prevents verify-tests-fail from seeing PR fix files as uncommitted during Gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restore the committed review branch before each gate verification attempt so a crashed without-fix attempt cannot leave fix files reverted and make the next retry fail with uncommitted changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Capture Copilot CLI usage metadata during review runs, aggregate it into all-stage summaries, and publish a dedicated CopilotTokenUsage artifact from ci-copilot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Enable the Copilot OTel file exporter per review step and merge exported token metrics into the existing CopilotTokenUsage artifact summaries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Summary
APPROVE,REQUEST_CHANGES, or safeCOMMENTfallback.SECTION:PR-FINALIZEblocks./review rerunafter new comments or commits when they want a fresh review. The command implementation is intentionally split into a follow-up PR.Validation
System.Management.Automation.Language.Parser..github/workflows/review-trigger.ymlas YAML.Invoke-Pester .github/scripts/Post-AISummaryComment.Tests.ps1,.github/scripts/Remove-StaleMauiBotComments.Tests.ps1 -CIDetect-TestsInDiff.ps1mapssrc/BlazorWebView/tests/MauiBlazorWebView.UnitTests/UriExtensions_Tests.cstosrc/BlazorWebView/tests/MauiBlazorWebView.UnitTests/MauiBlazorWebView.UnitTests.csproj.