Automate visual failure comparisons in /review tests

[kubaflo]

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!

Description of Change

Automates visual snapshot evidence in /review tests. When public AzDO results contain failed screenshot comparisons, the command now emits exactly one test-failure analysis comment containing bounded, expandable baseline/actual/diff panels.

Visual evidence remains supplementary: it does not change gate.verdictCeiling, deterministic attribution, or the merge-readiness verdict.

One-comment flow

1. Trusted pre-activation code discovers failed visual results through the public AzDO resultsbybuild API, including retry-suffixed attachments such as Snapshot[1].png and Snapshot-diff[1].png.
2. It resolves baselines from the exact source version tested by AzDO and maps runtime evidence to the correct snapshot directory (ios-26, android-notch-36, mac, or windows).
3. It streams and validates bounded PNG assets, then stores them on review-tests-assets using immutable commit-pinned raw.githubusercontent.com URLs.
4. The Copilot agent emits the normal single add_comment analysis payload with a trusted insertion marker.
5. A sealed post-step validates the published asset manifest and injects as many expandable comparison panels as fit into that same comment. Each collapsed panel shows a conservative relationship label:
   • Likely PR-caused for an exact test/platform base regression or directly changed snapshot/test;
   • Likely unrelated for an exact base/known-issue match without direct visual scope;
   • Needs human investigation for unmatched or mixed evidence.
6. Excess comparisons are summarized as omitted instead of creating a companion comment.

The local .github/scripts/Review-Tests.ps1 -PostComment path uses the same merger. It also recognizes complete reports returned in Copilot's final response, preserving nested evidence code fences without wrapping a second title or badge section.

Security and Failure Safety

• PR text, logs, test names, attachment metadata, changed files, and visual labels remain untrusted input.
• The merger script and visual context are copied to a root-owned location before the workflow checks out the untrusted PR branch.
• The post-step runs without COPILOT_GITHUB_TOKEN, GH_TOKEN, or GITHUB_TOKEN.
• AzDO attachment URLs must match the expected public project and attachment route.
• Published assets are size-bounded, signature-checked PNGs with validated dimensions and repository paths.
• Raw image URLs must match the exact repository, asset commit, PR directory, and safe filename.
• Labels are HTML-escaped and @ is neutralized before insertion.
• Relationship labels use fixed trusted text. Untrusted attribution values are never rendered, and same-named snapshots changed on another platform do not count as PR scope.
• The final body is checked panel-by-panel against conservative limits of 45 URLs, 10 mentions, and 60,000 UTF-16 characters, below gh-aw's throwing limits.
• The analysis JSON update is atomic (written to a temp file, then renamed over the original). Invalid context, malformed output, missing analysis payloads, limit failures, and dry-run/noop output leave the original analysis unchanged.
• The publisher never creates or patches PR comments; only the existing gh-aw add_comment payload is mutated.

What NOT to Do

• Do not use the ordinary anonymous AzDO test-runs listing for discovery; it redirects to sign-in. Use the public failed-results endpoint.
• Do not resolve baselines from the current PR head; use the source version actually tested by the selected AzDO build.
• Do not let the agent construct or trust visual asset URLs.
• Do not publish a second companion comment; merge bounded panels into the single analysis payload.

Validation

• 98 focused Pester tests pass.
• Changed PowerShell scripts parse successfully.
• gh aw compile copilot-review-tests --approve completes without errors or warnings.
• A real agent_output.json from gh-aw run 29674953402 was replayed through the post-step:
  • one add_comment item remained one item;
  • five visual panels were inserted;
  • the final body contained 26 URLs, one mention, and 9,585 characters.

Live Single-Comment Examples

The exact local /review tests path from this branch posted or repaired these merged comments after the PRs' /azp run pipelines completed:

PR	Single merged result	Included evidence	Relationship labels	Final limits
#36413	Test-failure analysis with visual panels	5 panels / 15 images	1 PR-caused, 4 investigate	23 URLs, 13,156 chars
#36631	Test-failure analysis with visual panels	6 panels / 18 images	2 PR-caused, 4 investigate	31 URLs, 21,367 chars
#36395	Test-failure analysis with visual panels	11 panels / 33 images; 19 omitted	11 investigate	43 URLs, 19,858 chars
#36404	Test-failure analysis with visual panels	14 panels / 40 images; 81 omitted	14 investigate	45 URLs, 23,779 chars
#35846	Test-failure analysis with visual panels	10 panels / 30 images; 9 omitted	10 investigate	43 URLs, 22,496 chars
#36277	Test-failure analysis with visual panels	7 panels / 19 images	3 PR-caused, 4 investigate	31 URLs, 18,355 chars
#36170	Test-failure analysis with visual panels	11 panels / 33 images; 8 omitted	11 investigate	44 URLs, 23,180 chars
#35578	Test-failure analysis with visual panels	12 panels / 36 images; 50 omitted	12 investigate	44 URLs, 25,443 chars
#36672	Test-failure analysis with visual panels	14 panels / 40 images; 9 omitted	14 investigate	45 URLs, 25,915 chars
#31755	Test-failure analysis with visual panels	12 panels / 36 images; 3 omitted	12 investigate	44 URLs, 22,728 chars
#34637	Test-failure analysis with visual panels	9 panels / 27 images; 81 omitted	9 investigate	43 URLs, 22,325 chars
#35156	Test-failure analysis with visual panels	2 panels / 6 images	1 PR-caused, 1 investigate	19 URLs, 11,911 chars
#35885	Test-failure analysis with no visual failures	0 panels / 0 images	No visual failures detected	8 URLs, 4,239 chars
#36577	Test-failure analysis with visual panels	1 panel / 3 images	1 investigate	44 URLs, 21,768 chars
#36212	Test-failure analysis with no visual failures	0 panels / 0 images	No visual failures detected	5 URLs, 5,982 chars

Each result contains one Tests Failure Analysis title and one merged review marker. Across 114 rendered panels, all 336 embedded image URLs returned HTTP 200. Seven panels were safely classified as likely PR-caused; no panel in this sample had enough exact evidence to be safely classified as likely unrelated, so the remaining 107 stayed at Needs human investigation. Another 260 comparisons were omitted safely by the comment limits.

The latest eight-example batch was regenerated concurrently, and #36672, #31755, #34637, #35156, #35885, #36577, and #36212 were added afterward. The current review-tests-assets head retains the full asset history.

The protected copilot-pat-pool environment rejects feature-branch workflow_dispatch runs before job execution. The live local-runner examples validate comment generation and asset publication, while the real gh-aw output replay validates the workflow post-step mutation without weakening that branch protection.

Issues Fixed

N/A - reviewer workflow enhancement.

[github-actions]

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36666

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36666"

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

Skill Validation Results

@kubaflo — new skill validation results are available based on this last commit: 3f85eca.
To request a fresh validation after new comments or commits, comment /evaluate-skills.

✅ Skill Validation Results — 3f85eca · Automate visual failure comparisons in /review tests · 2026-07-24T06:57:14Z

✅ Static Checks Passed

Skills: 23 | Eval specs linted: 11

Full lint output

── .github/skills/agentic-labeler/tests/eval.vally.yaml
npm warn deprecated prebuild-install@7.1.3: No longer maintained. Please contact the author of the relevant native addon; alternatives are available.
✔ .github/skills/agentic-labeler/tests/eval.vally.yaml is valid
── .github/skills/analyze-sessions/tests/eval.vally.yaml
✔ .github/skills/analyze-sessions/tests/eval.vally.yaml is valid
── .github/skills/code-review/tests/eval.capability.vally.yaml
✔ .github/skills/code-review/tests/eval.capability.vally.yaml is valid
── .github/skills/code-review/tests/eval.inline-findings.vally.yaml
✔ .github/skills/code-review/tests/eval.inline-findings.vally.yaml is valid
── .github/skills/code-review/tests/eval.trim-aot.vally.yaml
✔ .github/skills/code-review/tests/eval.trim-aot.vally.yaml is valid
── .github/skills/code-review/tests/eval.vally.yaml
✔ .github/skills/code-review/tests/eval.vally.yaml is valid
── .github/skills/code-review/tests/hermeticity.vally.yaml
✔ .github/skills/code-review/tests/hermeticity.vally.yaml is valid
── .github/skills/evaluate-pr-tests/tests/eval.vally.yaml
✔ .github/skills/evaluate-pr-tests/tests/eval.vally.yaml is valid
── .github/skills/pr-review/tests/eval.gh-auth.vally.yaml
✔ .github/skills/pr-review/tests/eval.gh-auth.vally.yaml is valid
── .github/skills/try-fix/tests/eval.vally.yaml
✔ .github/skills/try-fix/tests/eval.vally.yaml is valid
── .github/skills/verify-tests-fail-without-fix/tests/eval.vally.yaml
✔ .github/skills/verify-tests-fail-without-fix/tests/eval.vally.yaml is valid

⏭️ LLM Evaluation: Skipped

No changed skills with eval specs found.

🔍 Full results and investigation steps

[Copilot]

Pull request overview

This PR enhances the /review tests GitHub workflow by automatically collecting visual snapshot failure evidence from public AzDO endpoints, publishing bounded baseline/actual/diff PNGs to a durable repo branch, and surfacing them via a companion expandable PR comment that the main AI analysis links to (instead of embedding many image URLs).

Changes:

• Adds trusted pre-activation steps to gather context, publish visual comparison assets/comment (non-blocking), and pass context to the agent via a short-lived artifact.
• Introduces a new PowerShell publisher (Publish-TestVisualAssets.ps1) plus Pester coverage to validate URL/path safety, PNG bounds, and comment/update behavior.
• Updates the review skill prompt and CI facts docs to guide linking to visualAssets.commentUrl and keeping visuals out of the main safe-output constrained comment.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
.github/workflows/copilot-review-tests.md	Adds permissions + trusted pre-activation gather/publish steps; switches agent to consume a downloaded context artifact; updates prompt instructions to link to visual companion comment.
.github/workflows/copilot-review-tests.lock.yml	Regenerates compiled workflow to reflect the new pre-activation steps, permissions, and artifact download path wiring.
.github/skills/review-test-failures/SKILL.md	Documents new visualEvidence / visualAssets fields and instructs the agent to link to the companion comment without embedding image URLs.
.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1	Adds Pester tests for the publisher’s validation, markdown rendering, and “create vs update” comment behavior.
.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1	New publisher script: downloads bounded PNGs, resolves baselines from the tested commit, writes comparison panels, publishes assets to review-tests-assets, and posts/updates the companion comment.
.github/skills/review-test-failures/scripts/Gather-TestFailureContext.Tests.ps1	Adds tests for visual snapshot parsing, retry attachment selection, and environment directory hint extraction.
.github/skills/review-test-failures/scripts/Gather-TestFailureContext.ps1	Adds public resultsbybuild discovery + per-result detail/attachments inspection to populate visualEvidence in context output.
.github/scripts/Review-Tests.ps1	Updates local runner to optionally publish visual assets/comment before invoking the agent, and instructs linking to visualAssets.commentUrl.
.github/docs/maui-ci-facts.md	Documents the new /review tests visual evidence pipeline and its non-impact on deterministic verdict ceilings.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1:215

• Invoke-DownloadFile follows HTTP redirects without validating the final destination. Since the attachment/baseline URLs are treated as untrusted input, an allowed AzDO/GitHub URL could redirect to an unexpected host and bypass the host/path allow-list checks. Validate that any redirect stays on the original host (or explicitly allow-list the final host) before consuming the response body.

            $handler = [System.Net.Http.HttpClientHandler]::new()
            $handler.AllowAutoRedirect = $true
            $handler.MaxAutomaticRedirections = 5
            $client = [System.Net.Http.HttpClient]::new($handler)

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

[PureWeen]

Adversarial code review

Two warnings and two non-blocking operational concerns survived adversarial consensus:

• ⚠️ Regression — .github/scripts/Review-Tests.ps1:259 recognizes the standard agent report marker and returns the report before recomputing failure, baseline, regression, platform, commit, and author metadata from context.json. A normal local report can therefore publish agent-transcribed values that disagree with the deterministic context. Preserve the detailed report body, but regenerate its badge and attribution metadata from the context.
  Flagged by: 2/3 reviewers after dispute

• ⚠️ Logic — .github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1:227 treats any changed file named <ClassName>.cs as the exact visual test, without the platform/path constraint used for snapshot files. Duplicate class filenames or a different-platform edit can override decisive base attribution and label a pre-existing failure as likely PR-caused. Match the expected test path/platform and add a duplicate-basename negative test.
  Flagged by: 2/3 reviewers

• 💡 Error Handling — .github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1:576 overwrites context.json directly. An I/O failure during Set-Content can destroy the valid context while the continue-on-error workflow proceeds. Write to a same-directory temporary file, validate it, and atomically replace the destination.
  Flagged by: 2/3 reviewers after dispute

• 💡 Config Impact — .github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1:464 chains every publication onto review-tests-assets, but no retention or cumulative repository-size policy is wired in. Add an explicit lifecycle policy or use storage designed for retained visual assets.
  Flagged by: 2/3 reviewers

Test coverage: The focused Pester coverage is substantial, but it does not cover deterministic badge preservation or duplicate class basenames across platform paths.

Prior review status: Four existing Copilot inline findings were not duplicated and remain visible on the PR.

PR metadata: The title is accurate and the description is strong. The description claim that JSON updates are atomic does not currently match Save-Context.

What looks right: The trusted merger is sealed before PR checkout, and the post-step removes GitHub tokens before mutating the agent output.

Methodology: 3 independent reviewers with adversarial consensus.

[kubaflo]

Validated all four findings against HEAD (4ccbab1) — reply-only (this branch is checked out in another worktree, so I'm confirming rather than pushing). All four hold:

1. ⚠️ Regression — Review-Tests.ps1:259. Confirmed. In New-TestFailureReviewBody, when Get-EmbeddedTestFailureReport returns a report, the function return $completeReport early (L268), before the deterministic block below recomputes verdict/failure-count/commit/author from context. So an agent-transcribed badge/attribution is published without cross-checking context.json. Preserving the detailed body while regenerating badge + attribution from context is the right fix.

2. ⚠️ Logic — Merge-TestVisualsIntoComment.ps1:227. Confirmed. The first match path constrains by filename and Test-VisualSnapshotPathMatchesPlatform (L211-217), but the fallback class-name path (L225-232) matches any changed file whose GetFileName equals <ClassName>.cs with no platform/path constraint. A duplicate basename in another directory, or a different-platform edit, can override decisive base attribution and mislabel a pre-existing failure as PR-caused. Match the expected test path/platform and add a duplicate-basename negative test.

3. 💡 Error Handling — Publish-TestVisualAssets.ps1:576. Confirmed. Save-Context does ConvertTo-Json | Set-Content -LiteralPath $Path directly — a failure mid-write truncates the valid context while the continue-on-error workflow proceeds. Write to a same-directory temp file, validate, then atomically replace. (This also reconciles the PR-description "atomic JSON updates" claim, which Save-Context doesn't currently satisfy.)

4. 💡 Config Impact — Publish-TestVisualAssets.ps1:464. Confirmed. The commit loop chains each publication onto $Branch (parents = @($parentSha)) with no retention or cumulative-size policy, so review-tests-assets history grows unbounded. Add an explicit lifecycle policy or use storage designed for retained assets.

@PureWeen — thanks for the adversarial pass; findings 1 and 2 are the ones worth prioritizing.

🔍 AI-generated reply on @kubaflo's behalf.

[Copilot]

Copilot's findings

• Files reviewed: 13/13 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

• Files reviewed: 13/13 changed files
• Comments generated: 0 new

[Copilot]

Copilot's findings

• Files reviewed: 13/13 changed files
• Comments generated: 0 new

[Copilot]

Copilot's findings

• Files reviewed: 13/13 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

• Files reviewed: 13/13 changed files
• Comments generated: 0 new

[kubaflo]

@PureWeen addressed the latest Copilot finding in a122d990bd7: Test-AzDoAttachmentUrl (the attachment-download allowlist gate) now rejects URLs with userinfo, a non-default port, a query, or a fragment — only an unadorned https://dev.azure.com/<expected path> passes (explicit :443 stays accepted). Added rejection + default-port tests, verified fail-without-fix. Publish suite 19/0. Ready for re-review — thanks!

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

[kubaflo]

Good catch, and the suggested approach is right. For workflow_dispatch the pre-activation gates resolve true (is_team_member/command_position_ok), so Gather test-failure context runs; with an empty PR_NUMBER the set -euo pipefail + exit 1 ("PR number is required") is masked by continue-on-error: true, and the activated output still gates true → the agent job proceeds into a confusing no-op.

Concrete fix on the source step in copilot-review-tests.md (keeps resilience for /review tests, fails fast for manual dispatch):

    - name: Gather test-failure context
      ...
      continue-on-error: ${{ github.event_name != 'workflow_dispatch' }}

I'm intentionally not hand-applying this here: this .md is the gh-aw source and the runtime is the generated copilot-review-tests.lock.yml (138 KB), which was compiled with gh-aw v0.82.14. Editing only the .md would drift from the lock, and hand-editing the generated lock would drift from the compiler. My environment's gh aw is v0.80.9 and can't reproduce the v0.82.14 lock (it rejects the newer model: frontmatter), so this needs a gh aw compile copilot-review-tests on v0.82.14 to regenerate the lock cleanly. Flagging for that recompile rather than introducing source/lock drift. (Leaving the thread open for the owner to apply + compile.)

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

[PureWeen]

Adversarial review: no issues found

Three independent reviewers completed a full current-head review with adversarial consensus. No correctness, security, regression, data-loss, race, resource, timeout, error-handling, or shared-config findings remain.

What looks right: trusted visual inputs are sealed before untrusted checkout, post-processing runs without GitHub tokens, asset URLs and PNG payloads are tightly validated, all network/process work is bounded, and the gh-aw source/lock pair is synchronized.

Test coverage: focused Pester coverage exercises gathering, publication, merging, local posting, timeout/failure paths, environment provenance, marker neutralization, and comment limits; 155 tests pass.

Prior review status: earlier findings were addressed across the current commit series; this pass found no duplicate or unresolved code-review issue.

Methodology: 3 independent reviewers with adversarial consensus. Review event: COMMENT.

[kubaflo]

Deployment validation found a repository-configuration blocker unrelated to the workflow implementation. Both the #36756 rerun and the #36507 run were rejected before a runner started:

Branch "main" is not allowed to deploy to copilot-pat-pool due to environment protection rules.

main and net11.0 are protected by repository rulesets, but the environment is still configured as Protected branches only, which checks classic branch protection and does not recognize either branch here. Commit 3f85eca8cb8 updates the shared PAT-pool guidance. A repository administrator must change copilot-pat-pool to Selected branches and tags and add exact rules for main and net11.0; no additional /review tests workflow code is needed for this gate.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

.github/workflows/copilot-review-tests.md:126

• Gather test-failure context is continue-on-error: true, but workflow_dispatch allows inputs.pr_number to be omitted. In that case this step’s “PR number is required” failure is masked and the workflow continues into the agent job with an empty PR number, leading to confusing/no-op behavior (and potentially invalid add_comment targeting). Consider failing fast for workflow_dispatch while keeping resilience for /review tests comment-triggered runs (e.g., make continue-on-error conditional or require pr_number for dispatch).

    - name: Gather test-failure context
      if: >-
        steps.exact_command.outputs.should_run == 'true' &&
        steps.check_membership.outputs.is_team_member == 'true' &&
        steps.check_command_position.outputs.command_position_ok == 'true'
      # Resilience: a transient failure gathering context (AzDO/Helix/network) must
      # NOT fail the pre-activation job, otherwise the agent job — which is designed
      # to post a short failure report when the context files are missing (see the
      # prompt's pre-flight below) — is skipped entirely and the run goes silent.
      # The whole downstream already tolerates a missing context.json (the artifact
      # download is continue-on-error, the seal/merge steps exit 0 when it's absent).
      continue-on-error: true