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

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

@@ -15,29 +15,35 @@ 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.
 
-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.
 
 ## 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/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

.github/scripts/Apply-PRFinalize.Tests.ps1

@@ -10,6 +10,7 @@
 
 BeforeAll {
     $scriptPath = Join-Path $PSScriptRoot 'apply-pr-finalize.ps1'
+    $script:ScriptText = Get-Content -Raw -LiteralPath $scriptPath
     $tokens = $null
     $parseErrors = $null
     $ast = [System.Management.Automation.Language.Parser]::ParseFile($scriptPath, [ref]$tokens, [ref]$parseErrors)
@@ -38,6 +39,7 @@ BeforeAll {
         'Get-FinalizeRecommendation',
         'Merge-PreservedTitlePrefix',
         'Merge-PreservedBodyPreamble',
+        'New-PullRequestUpdatePayload',
         'New-ExclusiveTempFile'
     )) {
         $function = $ast.Find({
@@ -226,6 +228,37 @@ Old description.
     }
 }
 
+Describe 'New-PullRequestUpdatePayload' {
+    It 'includes only the title when only the title changed' {
+        $payload = New-PullRequestUpdatePayload `
+            -TitleChanged $true `
+            -Title '[Android] RadioButton: Clear reset borders' `
+            -BodyChanged $false `
+            -Body 'unchanged'
+
+        @($payload.Keys) | Should -Be @('title')
+        $payload.title | Should -Be '[Android] RadioButton: Clear reset borders'
+    }
+
+    It 'includes only the body when only the body changed' {
+        $payload = New-PullRequestUpdatePayload `
+            -TitleChanged $false `
+            -Title 'unchanged' `
+            -BodyChanged $true `
+            -Body "### Change`n`nUpdated details."
+
+        @($payload.Keys) | Should -Be @('body')
+        $payload.body | Should -Be "### Change`n`nUpdated details."
+    }
+
+    It 'uses the REST pull-request endpoint for reads and writes' {
+        $script:ScriptText | Should -Match ([regex]::Escape('$prOutput = @(& gh api "repos/$Repo/pulls/$PRNumber"'))
+        $script:ScriptText | Should -Match ([regex]::Escape('$ghArgs = @(''api'', "repos/$Repo/pulls/$PRNumber", ''--method'', ''PATCH'', ''--input'', $payloadFile, ''--silent'')'))
+        $script:ScriptText | Should -Not -Match '\bgh\s+pr\s+(?:view|edit)\b'
+        $script:ScriptText | Should -Not -Match ([regex]::Escape("@('pr', 'edit'"))
+    }
+}
+
 Describe 'ConvertTo-AzdoSafeConsole' {
     # Behaviour is pinned to the canonical implementation in Review-PR.ps1; these mirror the
     # assertions in Review-PR.Tests.ps1 so the duplicated copy can't silently drift.

.github/scripts/BuildAndRunHostApp.Tests.ps1

@@ -0,0 +1,83 @@
+#Requires -Modules Pester
+
+# Focused tests for the Android per-test flaky-retry classification in
+# BuildAndRunHostApp.ps1: "Baseline snapshot not yet created" failures are
+# brand-new VerifyScreenshot tests (no committed baseline). They are
+# deterministic new-baseline results, NOT emulator flake, and must be excluded
+# from the flaky-retry set (retrying them wastes a full re-run and can exhaust
+# the deep category time budget on snapshot-heavy PRs).
+#
+# The retry logic is embedded in a large script rather than a callable function,
+# so these tests exercise the exact classification predicate used there.
+
+Describe 'Android flaky-retry new-baseline exclusion' {
+    BeforeAll {
+        $script:BaselineRegex = '(?i)Baseline snapshot not yet created'
+
+        # Mirrors the predicate in BuildAndRunHostApp.ps1: given TRX-style
+        # results ({ status; name; error }), return the names to retry
+        # (Failed and NOT a new-baseline failure).
+        function Get-RetryNames {
+            param([object[]]$Results)
+            $failed = @($Results | Where-Object { $_.status -eq 'Failed' })
+            @($failed |
+                Where-Object { ($_.error -as [string]) -notmatch $script:BaselineRegex } |
+                ForEach-Object { $_.name })
+        }
+
+        Describe 'MacCatalyst Apple Account dialog dismissal' {
+            It 'prefers the trusted staged script location before the repository fallback' {
+                $scriptContent = Get-Content (Join-Path $PSScriptRoot 'BuildAndRunHostApp.ps1') -Raw
+                $trustedPath = '../eng-scripts/dismiss-apple-account-dialog.sh'
+                $fallbackPath = '../../eng/scripts/dismiss-apple-account-dialog.sh'
+
+                $scriptContent.IndexOf($trustedPath) | Should -BeLessThan $scriptContent.IndexOf($fallbackPath)
+            }
+        }
+
+        function Get-BaselineCount {
+            param([object[]]$Results)
+            @($Results | Where-Object {
+                $_.status -eq 'Failed' -and (($_.error -as [string]) -match $script:BaselineRegex)
+            }).Count
+        }
+    }
+
+    It 'excludes baseline-not-created failures from the retry set' {
+        $results = @(
+            [pscustomobject]@{ status='Failed'; name='SearchBar_Material3_A'; error='Baseline snapshot not yet created: /snapshots/android/SearchBar_A.png' }
+            [pscustomobject]@{ status='Failed'; name='SearchBar_Material3_B'; error='Baseline snapshot not yet created: /snapshots/android/SearchBar_B.png' }
+            [pscustomobject]@{ status='Passed'; name='Switch_C'; error='' }
+        )
+        (Get-RetryNames -Results $results).Count | Should -Be 0
+        Get-BaselineCount -Results $results | Should -Be 2
+    }
+
+    It 'keeps genuine (non-baseline) flaky failures in the retry set' {
+        $results = @(
+            [pscustomobject]@{ status='Failed'; name='Flaky_Timeout'; error='System.TimeoutException: element not found' }
+            [pscustomobject]@{ status='Failed'; name='New_Snapshot'; error='Baseline snapshot not yet created: /snapshots/android/New.png' }
+            [pscustomobject]@{ status='Passed'; name='Ok_Test'; error='' }
+        )
+        $retry = Get-RetryNames -Results $results
+        $retry | Should -Contain 'Flaky_Timeout'
+        $retry | Should -Not -Contain 'New_Snapshot'
+        $retry.Count | Should -Be 1
+        Get-BaselineCount -Results $results | Should -Be 1
+    }
+
+    It 'is case-insensitive on the baseline signature' {
+        $results = @(
+            [pscustomobject]@{ status='Failed'; name='X'; error='BASELINE SNAPSHOT NOT YET CREATED: /p.png' }
+        )
+        (Get-RetryNames -Results $results).Count | Should -Be 0
+        Get-BaselineCount -Results $results | Should -Be 1
+    }
+
+    It 'treats null/empty error as a retryable (non-baseline) failure' {
+        $results = @(
+            [pscustomobject]@{ status='Failed'; name='NoErrText'; error=$null }
+        )
+        (Get-RetryNames -Results $results) | Should -Contain 'NoErrText'
+    }
+}