Improve maui-copilot reviewer: gate & deep-stage reliability fixes

.github/docs/pr-review-workflow.md

@@ -63,6 +63,8 @@ The trigger is implemented by `.github/workflows/review-trigger.yml`. It:
 
 The workflow intentionally does not handle `/review tests`; that subcommand is reserved for the test-failure review workflow.
 
+GitHub Actions webhook deliveries can occasionally be delayed or dropped during an Actions incident. A deterministic scheduled fallback (`.github/workflows/review-trigger-recovery.yml`) polls recent commands, waits 25 minutes so both bounded trigger jobs have time to finish, rechecks the commenter's current repository permission, and dispatches the same trusted review workflow. The default-branch commit used by the first scheduled run is a permanent lower bound, preventing already-handled commands from being replayed when the fallback is introduced. Processed commands are marked so a delayed webhook cannot trigger a duplicate review.
+
 **Note**: Command comments are minimized (collapsed as "Resolved") after authorization to reduce conversation clutter while preserving the comment history. Unauthorized or malformed command comments remain fully visible.
 
 ### Platform inference
@@ -262,7 +264,7 @@ Important safeguards:
 
 | Symptom | Likely cause | What to do |
 | --- | --- | --- |
-| `/review` does nothing | The commenter does not have write/maintain/admin access, or the comment is not on a PR. | Ask a maintainer to run the command on the PR. |
+| `/review` does nothing | The commenter does not have write/maintain/admin access, the comment is not on a PR, or GitHub Actions delayed the webhook. | Authorized commands should be recovered automatically within about 35 minutes. Check GitHub Status if Actions is degraded. |
 | `/review` used the wrong platform | Platform labels were missing or ambiguous. | Re-run with an explicit platform, for example `/review ios`. |
 | `/review tests` says `Insufficient data` | Build/log/Helix evidence was inaccessible or incomplete. | Re-run later, provide a build ID, or run locally with Azure CLI/AzDO auth. |
 | The AI Summary looks stale | New commits or author comments landed after the last review. | Wait for the automatic rerun queue, or ask a maintainer to run `/review` for an immediate review. |
@@ -272,6 +274,8 @@ Important safeguards:
 ## Related files
 
 - `.github/workflows/review-trigger.yml` — GitHub comment trigger for `/review`.
+- `.github/workflows/review-trigger-recovery.yml` — scheduled fallback for missed `/review` webhooks.
+- `.github/scripts/Recover-MissedReviewCommands.ps1` — deterministic recovery and duplicate-prevention logic.
 - `eng/pipelines/ci-copilot.yml` — Azure DevOps PR review pipeline.
 - `.github/scripts/Review-PR.ps1` — local script orchestrating full PR review phases.
 - `.github/scripts/post-ai-summary-comment.ps1` — AI Summary comment formatter.

.github/instructions/ci-copilot-pipeline-security.instructions.md

@@ -1,6 +1,6 @@
 ---
 description: "Security rules for the Copilot PR-review pipeline. Read before editing."
-applyTo: "eng/pipelines/ci-copilot.yml,eng/scripts/detect-ui-test-categories.ps1,.github/scripts/**,.github/pr-review/**,.github/skills/pr-review/**,.github/skills/verify-tests-fail-without-fix/**,.github/skills/try-fix/**,.github/skills/run-device-tests/**,.github/workflows/review-trigger.yml,.github/workflows/pr-review-queue.yml,.github/workflows/copilot-evaluate-tests.*"
+applyTo: "eng/pipelines/ci-copilot.yml,eng/scripts/detect-ui-test-categories.ps1,.github/scripts/**,.github/pr-review/**,.github/skills/pr-review/**,.github/skills/verify-tests-fail-without-fix/**,.github/skills/try-fix/**,.github/skills/run-device-tests/**,.github/workflows/review-trigger.yml,.github/workflows/review-trigger-recovery.yml,.github/workflows/pr-review-queue.yml,.github/workflows/copilot-evaluate-tests.*"
 ---
 
 # CI Copilot pipeline — security rules
@@ -15,29 +15,37 @@ Once the PR is merged into the worktree, the author controls every `.csproj`, `D
 
 ## Rules
 
-1. **Per-task `env:` scoping.** Only put tokens a task needs. The Copilot-agent task gets `COPILOT_GITHUB_TOKEN` only — never `GH_TOKEN`. Pass `--secret-env-vars=GH_TOKEN,GITHUB_TOKEN,COPILOT_GITHUB_TOKEN` to the Copilot CLI.
+1. **Per-task `env:` scoping.** Only put tokens in tasks that need them. The Copilot-agent task gets `COPILOT_GITHUB_TOKEN` only — never `GH_TOKEN`. The Post task runs in its own Microsoft-hosted job and receives `GH_COMMENT_TOKEN` only in its posting step. Pass `--secret-env-vars=GH_TOKEN,GITHUB_TOKEN,COPILOT_GITHUB_TOKEN` to the Copilot CLI.
 
-2. **`persistCredentials: false` on every `checkout: self`** unless the task pushes. Default checkout writes the service-connection PAT into `.git/config` as `extraheader`, readable by any subprocess.
+2. **`persistCredentials: false` on every `checkout: self`** unless the task pushes. Default checkout writes the service-connection PAT into `.git/config` as `extraheader`, readable by any subprocess. The trusted Stage 3 summary job is the explicit exception: it never runs PR-controlled code and scopes that credential to snapshot-asset publication and the conservative PR title/body updater.
 
-3. **Trusted-copy scripts before merging the PR.** Setup task (still on `main`) copies `.github/scripts`, `.github/skills`, `eng/scripts` to `$(Build.ArtifactStagingDirectory)/trusted-github/`, then `chmod -R a-w`. Later tasks invoke scripts from `$TRUSTED/...`, never from the merged worktree. In PowerShell use `$ScriptsDir` / `$SkillsDir` / `$EngScriptsDir` (canonical impl in `Review-PR.ps1`). New post-merge scripts must be added to the Setup copy block.
+3. **Trusted-copy scripts before merging the PR.** Setup copies `.github/scripts`, `.github/skills`, and `eng/scripts` to `$(Build.ArtifactStagingDirectory)/trusted-github/` before switching branches or merging the PR. Gate and CopilotReview invoke scripts through `$ScriptsDir`, `$SkillsDir`, and `$EngScriptsDir`, never from the merged worktree. New scripts used by those phases must be added to the Setup copy block.
 
-4. **Strip tokens before invoking PR-controlled code.** Wrap every `dotnet build|test|run|pack`, `msbuild`, `dotnet cake`, `BuildAndRun*.ps1`, `Run-DeviceTests.ps1`, `Invoke-UITestWithRetry.ps1` in `Invoke-WithoutGhTokens { ... }` (defined in `Review-PR.ps1` and `verify-tests-fail.ps1` — saves/clears/restores `GH_TOKEN`, `GITHUB_TOKEN`, `COPILOT_GITHUB_TOKEN`). **Wrap as close to the subprocess as possible, not at the outer trusted-script boundary** — a trusted script may itself need `gh` for metadata (e.g., `verify-tests-fail.ps1` calls `Detect-TestsInDiff.ps1` which uses `gh api`), so wrapping the whole script breaks its detection path. Wrap only the line that launches the PR-controlled process. Exception: scripts that ONLY call `gh` for PR metadata (`Detect-TestsInDiff.ps1`, `Find-RegressionRisks.ps1`, `detect-ui-test-categories.ps1`) don't need wrapping at all — they keep the token.
+4. **Run Post from a clean pipeline checkout.** Post runs in a separate Microsoft-hosted job, checks out `$(Build.SourceVersion)` with `clean: true` and `persistCredentials: false`, and executes `.github/scripts`, `.github/skills`, and `eng/scripts` from that checkout. It downloads review results separately and copies only `CustomAgentLogsTmp` into the expected data path. Do not copy artifact content over script directories.
 
-5. **Cross-phase signal files in `$(Agent.TempDirectory)`** (or `$TRUSTED`), never `$RepoRoot/...`. PR code can overwrite anything in the worktree, including a gate verdict. Readers must not silently fall back to a worktree path if the trusted one is missing.
+5. **Strip tokens before invoking PR-controlled code.** Wrap every `dotnet build|test|run|pack`, `msbuild`, `dotnet cake`, `BuildAndRun*.ps1`, `Run-DeviceTests.ps1`, `Invoke-UITestWithRetry.ps1` in `Invoke-WithoutGhTokens { ... }` (defined in `Review-PR.ps1` and `verify-tests-fail.ps1` — saves/clears/restores `GH_TOKEN`, `GITHUB_TOKEN`, `COPILOT_GITHUB_TOKEN`). **Wrap as close to the subprocess as possible, not at the outer trusted-script boundary** — a trusted script may itself need `gh` for metadata (e.g., `verify-tests-fail.ps1` calls `Detect-TestsInDiff.ps1` which uses `gh api`), so wrapping the whole script breaks its detection path. Wrap only the line that launches the PR-controlled process. Exception: scripts that ONLY call `gh` for PR metadata (`Detect-TestsInDiff.ps1`, `Find-RegressionRisks.ps1`, `detect-ui-test-categories.ps1`) don't need wrapping at all — they keep the token.
 
-6. **Strip `##vso[...]` from PR-controlled stdout.** Pipe through `tr -d '\r' | sed -E 's/##vso\[[^]]*\]//g'` — bare `sed` misses CRLF lines and the agent will execute the directive.
+6. **Cross-phase and cross-job results.** Same-job phase files belong in `$(Agent.TempDirectory)` or the trusted staging directory, never the merged worktree. Cross-job values use named output variables with a fixed set of expected values or pipeline artifacts. Download artifacts outside the checkout, copy only the required data directory, and never transfer scripts for Post to run.
 
-7. **`gh-aw` workflows.** Pin compiler version (≥ v0.68.4 strips `pull-requests: write` per `gh-aw#28767`). Regenerate `.lock.yml` with `gh aw compile` in the **same commit** as any `.md` frontmatter edit (stale lock ⇒ all dispatches fail). `workflow_dispatch` triggers must restore trusted `.github/` from main (see `Checkout-GhAwPr.ps1`).
+7. **Strip `##vso[...]` from PR-controlled stdout.** Pipe through `tr -d '\r' | sed -E 's/##vso\[[^]]*\]//g'` — bare `sed` misses CRLF lines and the agent will execute the directive.
 
-8. **No token republish.** Don't `setvariable` a token (visible to every later task, even with `issecret=true`). Don't write tokens to worktree files. Don't echo token names.
+8. **`gh-aw` workflows.** Pin compiler version (≥ v0.68.4 strips `pull-requests: write` per `gh-aw#28767`). Regenerate `.lock.yml` with `gh aw compile` in the **same commit** as any `.md` frontmatter edit (stale lock ⇒ all dispatches fail). `workflow_dispatch` triggers must restore trusted `.github/` from main (see `Checkout-GhAwPr.ps1`).
+
+9. **No token republish.** Don't `setvariable` a token (visible to every later task, even with `issecret=true`). Don't write tokens to worktree files. Don't echo token names.
+
+10. **Missed-command recovery stays trusted and deterministic.** The scheduled recovery workflow must execute only from the default branch, never check out PR code, revalidate the commenter's current write access, and dispatch the existing trusted review workflow rather than calling AzDO directly. Its minimum command age must exceed the combined timeout of the normal trigger jobs so polling cannot race an in-progress webhook delivery into a duplicate review.
 
 ## Review checklist
 
 - [ ] New `checkout: self` has `persistCredentials: false`.
 - [ ] New `env:` block lists only the tokens that task needs; Copilot task has no `GH_TOKEN`.
-- [ ] New post-merge script invoked via `$ScriptsDir` / `$SkillsDir` / `$EngScriptsDir`, not `$RepoRoot/...`, AND added to Setup copy block.
+- [ ] New Gate or CopilotReview script is invoked through `$ScriptsDir` / `$SkillsDir` / `$EngScriptsDir` and is included in the Setup copy block.
+- [ ] Post runs in its own Microsoft-hosted job from a clean checkout of the pipeline revision.
+- [ ] Post executes scripts from the checkout and copies only expected result data from pipeline artifacts.
+- [ ] Artifact content cannot overwrite `.github/scripts`, `.github/skills`, or `eng/scripts`.
+- [ ] Gate labels use the fixed `RunGate.gateResult` output.
 - [ ] New invocation of PR-controlled code (`dotnet test|build|run`, `BuildAndRun*`, `Run-DeviceTests`, `Invoke-UITestWithRetry`) is wrapped in `Invoke-WithoutGhTokens` AT THE CALL SITE (not at an outer boundary).
-- [ ] New cross-phase state file lives under `$(Agent.TempDirectory)` / `$TRUSTED`.
+- [ ] New same-job phase state uses `$(Agent.TempDirectory)` / trusted staging; new cross-job state uses an output variable or pipeline artifact.
 - [ ] New PR-stdout pipe uses `tr -d '\r' | sed -E 's/##vso\[[^]]*\]//g'`.
 - [ ] Edited `.github/workflows/*.md` has matching `.lock.yml` regenerated in same commit.
 

.github/patches/catalyst-retina-screenshot.patch

@@ -0,0 +1,109 @@
+diff --git a/src/Controls/tests/TestCases.Shared.Tests/UITest.cs b/src/Controls/tests/TestCases.Shared.Tests/UITest.cs
+--- a/src/Controls/tests/TestCases.Shared.Tests/UITest.cs
++++ b/src/Controls/tests/TestCases.Shared.Tests/UITest.cs
+@@ -1,4 +1,5 @@
+ using System.Reflection;
++using System.Runtime.InteropServices;
+ using System.Text.RegularExpressions;
+ using ImageMagick;
+ using ImageMagick.Drawing;
+@@ -654,6 +655,35 @@ namespace Microsoft.Maui.TestCases.Tests
+ 		}
+
+ #if MACUITEST
++		const string CoreGraphicsLibrary = "/System/Library/Frameworks/CoreGraphics.framework/CoreGraphics";
++
++		[StructLayout(LayoutKind.Sequential)]
++		struct NativePoint
++		{
++			public double X;
++			public double Y;
++		}
++
++		[StructLayout(LayoutKind.Sequential)]
++		struct NativeSize
++		{
++			public double Width;
++			public double Height;
++		}
++
++		[StructLayout(LayoutKind.Sequential)]
++		struct NativeRectangle
++		{
++			public NativePoint Origin;
++			public NativeSize Size;
++		}
++
++		[DllImport(CoreGraphicsLibrary)]
++		static extern uint CGMainDisplayID();
++
++		[DllImport(CoreGraphicsLibrary)]
++		static extern NativeRectangle CGDisplayBounds(uint display);
++
+ 		byte[] TakeScreenshot()
+ 		{
+ 			// Since the Appium screenshot on Mac (unlike Windows) is of the entire screen, not just the app,
+@@ -672,16 +702,57 @@ namespace Microsoft.Maui.TestCases.Tests
+ 			if (width <= 0 || height <= 0)
+ 				return bytes;
+
+-			// Draw a rounded rectangle with the app window bounds as mask
+-			using var surface = new MagickImage(MagickColors.Transparent, (uint)width, (uint)height);
++			using var image = new MagickImage(bytes);
++			var displayBounds = CGDisplayBounds(CGMainDisplayID());
++
++			if (displayBounds.Size.Width <= 0 || displayBounds.Size.Height <= 0)
++				throw new InvalidOperationException($"Invalid main display bounds: {displayBounds.Size.Width}x{displayBounds.Size.Height}.");
++
++			// Mac2 reports element bounds in logical points while screenshots use
++			// display pixels. Hosted macOS agents currently use Retina scaling, so
++			// applying point coordinates directly crops the wrong part of the desktop.
++			double scaleX = image.Width / displayBounds.Size.Width;
++			double scaleY = image.Height / displayBounds.Size.Height;
++
++			if (!double.IsFinite(scaleX) || !double.IsFinite(scaleY) || scaleX <= 0 || scaleY <= 0)
++				throw new InvalidOperationException($"Invalid Mac screenshot scale: {scaleX}x{scaleY}.");
++
++			int pixelX = (int)Math.Round((x - displayBounds.Origin.X) * scaleX);
++			int pixelY = (int)Math.Round((y - displayBounds.Origin.Y) * scaleY);
++			int pixelRight = (int)Math.Round((x + width - displayBounds.Origin.X) * scaleX);
++			int pixelBottom = (int)Math.Round((y + height - displayBounds.Origin.Y) * scaleY);
++			int pixelWidth = pixelRight - pixelX;
++			int pixelHeight = pixelBottom - pixelY;
++
++			if (pixelX < 0 || pixelY < 0 || pixelWidth <= 0 || pixelHeight <= 0 ||
++				pixelRight > image.Width || pixelBottom > image.Height)
++			{
++				throw new InvalidOperationException(
++					$"Mac app window pixels ({pixelX},{pixelY},{pixelWidth},{pixelHeight}) " +
++					$"are outside screenshot bounds {image.Width}x{image.Height}.");
++			}
++
++			int pixelCornerRadius = Math.Max(1, (int)Math.Round(cornerRadius * Math.Min(scaleX, scaleY)));
++
++			// Draw a rounded rectangle with the physical-pixel app window bounds as mask.
++			using var surface = new MagickImage(MagickColors.Transparent, (uint)pixelWidth, (uint)pixelHeight);
+ 			new Drawables()
+-				.RoundRectangle(0, 0, width, height, cornerRadius, cornerRadius)
++				.RoundRectangle(0, 0, pixelWidth, pixelHeight, pixelCornerRadius, pixelCornerRadius)
+ 				.FillColor(MagickColors.Black)
+ 				.Draw(surface);
+
+-			// Composite the screenshot with the mask
+-			using var image = new MagickImage(bytes);
+-			surface.Composite(image, -x, -y, CompositeOperator.SrcAtop);
++			surface.Composite(image, -pixelX, -pixelY, CompositeOperator.SrcAtop);
++
++			// Keep committed snapshots density-independent and preserve the existing
++			// logical crop values (for example, the 29-point title-bar crop).
++			if (pixelWidth != width || pixelHeight != height)
++			{
++				var logicalSize = new MagickGeometry((uint)width, (uint)height)
++				{
++					IgnoreAspectRatio = true,
++				};
++				surface.Resize(logicalSize);
++			}
+
+ 			return surface.ToByteArray(MagickFormat.Png);
+ 		}

.github/pr-review/pr-report.md

@@ -12,8 +12,9 @@
 
 - Phases 1-2 (Pre-Flight, Try-Fix) must be complete before starting
 - Gate result is available from the prompt (ran separately before this skill)
-- **Read `pre-flight/content.md`** to get the code-review summary (verdict, confidence, error/warning counts)
-- Optionally read `pre-flight/code-review.md` for full findings if needed for the recommendation
+- **Read `pre-flight/content.md`** for issue/PR context
+- **Read `expert-pr-eval/content.md`** for the code-review verdict, confidence, and findings
+- Read `try-fix/content.md` and the individual candidate outputs for the comparison
 
 ---
 
@@ -25,11 +26,11 @@
    |----------|-----------|----------------|
    | 1 | Code review verdict is `NEEDS_CHANGES` (any ❌ errors) | `⚠️ REQUEST CHANGES` — code review found errors |
    | 2 | Gate failed (tests fail with fix) | `⚠️ REQUEST CHANGES` — fix doesn't work |
-   | 3 | Alternative fix found via Try-Fix that is simpler/better | `⚠️ REQUEST CHANGES` — suggest alternative |
+   | 3 | `pr-plus-reviewer` or a `try-fix-*` candidate wins | `⚠️ REQUEST CHANGES` — submitted PR needs the winning changes |
    | 4 | Code review verdict is `NEEDS_DISCUSSION` | `⚠️ REQUEST CHANGES` — include code review concerns |
-   | 5 | PR's fix selected AND Gate passed AND code review LGTM or SKIPPED | `✅ APPROVE` |
+   | 5 | Raw `pr` candidate wins AND Gate permits approval AND code review is LGTM or SKIPPED | `✅ APPROVE` |
 
-   **🚨 Hard gate:** If the code review (from Pre-Flight) has verdict `NEEDS_CHANGES`, the final recommendation MUST be `REQUEST CHANGES` regardless of Gate or Try-Fix results. Code-review ❌ Errors cannot be overridden by passing tests alone.
+   **🚨 Hard gate:** If the expert code review has verdict `NEEDS_CHANGES`, the final recommendation MUST be `REQUEST CHANGES` regardless of Gate or Try-Fix results. Code-review ❌ Errors cannot be overridden by passing tests alone.
 
    **Code review SKIPPED:** If the code-review sub-agent failed or timed out (verdict = `SKIPPED`), the hard gate does NOT apply. Proceed as if code review was not available — base the recommendation on Gate and Try-Fix results only. Note in the report that code review was unavailable.
 
@@ -47,7 +48,7 @@
 mkdir -p CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/report
 ```
 
-Write `content.md`:
+Write `content.md`. Its first non-empty line must be exactly the canonical heading shown below:
 ```markdown
 ## {✅/⚠️} Final Recommendation: {APPROVE/REQUEST CHANGES}
 
@@ -60,8 +61,8 @@ Write `content.md`:
 | Try-Fix | ✅ COMPLETE | {N} attempts, {M} passing |
 | Report | ✅ COMPLETE | |
 
-### Code Review Impact on Try-Fix
-{Brief description of how code-review findings influenced try-fix exploration. Did any model specifically address a code review ❌ Error? Did failure-mode probes reveal issues that guided fix approaches?}
+### Code Review and Candidate Comparison
+{Briefly identify which candidates address the expert review findings and whether any candidate leaves a ❌ Error unresolved. Do not imply the expert pass influenced earlier try-fix attempts; it runs after those attempts.}
 
 ### Summary
 {Brief summary of the review}
@@ -96,6 +97,7 @@ Standard markers in content.md: `✅ PASSED`, `❌ FAILED`, `Selected Fix: PR`,
 
 ## Common Mistakes
 
+- ❌ Replacing the required first line with `## Result`, `**Winner:**`, or equivalent prose
 - ❌ Rushing the report — take time for clear justification
 - ❌ Running git commands — user handles commit/push
 - ❌ Posting comments — this phase only produces output files, never posts to GitHub