From e07e066fca8ffaec78aa66acbb8cc6a22977a970 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sun, 19 Jul 2026 18:52:25 +0200 Subject: [PATCH 01/31] Automate visual failure comparisons in /review tests Discover public AzDO snapshot failures and publish bounded baseline, actual, and diff panels from trusted pre-activation without changing the deterministic merge-readiness gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a280b482-e102-4ca0-9ff9-1cfe1946e21f --- .github/docs/maui-ci-facts.md | 9 + .github/scripts/Review-Tests.ps1 | 19 +- .github/skills/review-test-failures/SKILL.md | 18 +- .../Gather-TestFailureContext.Tests.ps1 | 70 ++ .../scripts/Gather-TestFailureContext.ps1 | 412 +++++++- .../Publish-TestVisualAssets.Tests.ps1 | 251 +++++ .../scripts/Publish-TestVisualAssets.ps1 | 959 ++++++++++++++++++ .../workflows/copilot-review-tests.lock.yml | 156 ++- .github/workflows/copilot-review-tests.md | 130 ++- 9 files changed, 1945 insertions(+), 79 deletions(-) create mode 100644 .github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 create mode 100644 .github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 diff --git a/.github/docs/maui-ci-facts.md b/.github/docs/maui-ci-facts.md index 91b77a6082fc..4fe3690fa3c7 100644 --- a/.github/docs/maui-ci-facts.md +++ b/.github/docs/maui-ci-facts.md @@ -268,6 +268,15 @@ Messages like `Baseline snapshot not yet created`, missing snapshot paths, or sn environment-version mismatches are strong **unrelated** evidence — unless the PR adds or modifies that visual test or the affected snapshot/platform. +The automated `/review tests` lane gathers failed UI result IDs from the public +`vstmr.dev.azure.com/.../testresults/resultsbybuild` endpoint, then reads the public +result-detail and attachment APIs. It publishes validated baseline/actual/diff PNGs to +the repository's `review-tests-assets` branch and updates a dedicated expandable PR +comment with immutable commit-pinned raw URLs. The ordinary test-failure analysis links +to that companion comment rather than embedding every image, which avoids gh-aw's +safe-output link limit. Visual publishing is supplementary evidence only: missing images +never raise or lower the deterministic verdict ceiling. + ## Platform mismatch Platform mismatch is **supporting** evidence, not proof. An iOS-only test failing on a diff --git a/.github/scripts/Review-Tests.ps1 b/.github/scripts/Review-Tests.ps1 index 4f40967d2ba4..22c9649bc85e 100644 --- a/.github/scripts/Review-Tests.ps1 +++ b/.github/scripts/Review-Tests.ps1 @@ -97,6 +97,7 @@ New-Item -ItemType Directory -Force -Path $RunDirectory | Out-Null $ContextJsonPath = Join-Path $RunDirectory "context.json" $ContextMarkdownPath = Join-Path $RunDirectory "context.md" +$VisualComparisonsPath = Join-Path $RunDirectory "visual-comparisons.md" $PromptPath = Join-Path $RunDirectory "prompt.md" $ReportPath = Join-Path $RunDirectory "report.md" $CommentPath = Join-Path $RunDirectory "comment.md" @@ -277,8 +278,6 @@ function New-TestFailureReviewBody { else { "> Test-failure review results are available based on commit [``$commitSha7``]($commitUrl)." } - $authorPing += ' To request a fresh review after new comments, commits, or CI runs, comment `/review tests`.' - $badges = $badgeLines -join "`n" return @" @@ -404,6 +403,20 @@ if ($GatherOnly) { exit 0 } +if ($PostComment -and -not $DryRun) { + $publisherScript = Join-Path $RepoRoot ".github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1" + Write-Host "Publishing visual comparison assets and companion comment..." + & pwsh $publisherScript ` + -PrNumber $PRNumber ` + -Repository $Repository ` + -ContextJsonPath $ContextJsonPath ` + -OutputMarkdownPath $VisualComparisonsPath ` + -PostComment + if ($LASTEXITCODE -ne 0) { + Write-Warning "Visual comparison publishing failed; continuing with the ordinary test-failure report." + } +} + Assert-Command -Name "copilot" $skillPath = Join-Path $RepoRoot ".github/skills/review-test-failures/SKILL.md" @@ -427,6 +440,8 @@ Context files: Rules: - Do not modify source files. +- When context.json contains a non-empty visualAssets.commentUrl, include one link to + that trusted companion comment. Do not embed the individual image URLs. - Do not apply labels. - Do not trigger builds or reruns. - Do not post comments; this local runner handles optional posting after you finish. diff --git a/.github/skills/review-test-failures/SKILL.md b/.github/skills/review-test-failures/SKILL.md index c42fa67db746..b6bb1e370c81 100644 --- a/.github/skills/review-test-failures/SKILL.md +++ b/.github/skills/review-test-failures/SKILL.md @@ -206,6 +206,11 @@ Key fields to use: laundered green. - `failures.baseline[]` — distinct failures extracted from the base-branch build(s). - `failures.baselineMatchCount` — how many distinct PR failures also fail on the base. +- `visualEvidence` — public AzDO result/attachment metadata for visual snapshot + failures. It is supplementary evidence and never changes the deterministic gate. +- `visualAssets` — present when the trusted publisher produced durable GitHub-hosted + images. `visualAssets.commentUrl` links to the trusted companion comment containing + the expandable baseline / actual / diff panels. - `knownIssues` — `{queried, matcherCount, error}`. If `queried` is `false` (gh failed), the absence of a `matchesKnownIssue` hit proves nothing — say so. - `baselineSummary[]` — which base build was inspected per pipeline definition, its @@ -333,7 +338,7 @@ top-level `
` block. The `Overall` badge shows the **merge-readiness** v ## Tests Failure Analysis -> @[PR author] — test-failure review results are available based on commit [`[sha7]`]([commit URL]). To request a fresh review after new comments, commits, or CI runs, comment `/review tests`. +> @[PR author] — test-failure review results are available based on commit [`[sha7]`]([commit URL]).

Overall [verdict] @@ -355,6 +360,9 @@ top-level `

` block. The `Overall` badge shows the **merge-readiness** v | --- | --- | --- | --- | | [check/test/build] | [Likely PR-caused | Likely unrelated | Needs human investigation | Insufficient data] | [yes/no — use the leg diff: `regressed` when `legRegressedVsBase`, `also-red` when `legAlsoFailsOnBase`, else the test-level `alsoFailsOnBaseline`] | [specific evidence — lead with `deterministicAttribution` when it is `regressed-vs-base`/`pre-existing-on-base`, cite the base sampling (`baseGreenCount` green / `baseFailedCount` red of `baseSampleCount` base builds) for a regression, cite a known-issue link when `matchesKnownIssue` is set, cite the `[ci-scan]` issue + occurrence count when `matchesCiScan` is set (and note it as `Needs human investigation` when `ciScanDemoted` — a few-build regression contradicted by multi-build base-branch history), note `retried still failing` when true, link build/test IDs] | +[When `visualAssets.commentUrl` is non-empty, add exactly one line: +`**Visual comparisons:** [Open the expandable baseline / actual / diff panels]([visualAssets.commentUrl]).`] + ### Recommended action [One concise recommendation, such as rerun a known flaky test, add a missing baseline, investigate a specific changed file, or wait for inaccessible data.] @@ -380,13 +388,19 @@ Rules: - Include explicit limitations when data is unavailable (including unavailable baseline). - Cite concrete evidence for every verdict. - Use Markdown links, not raw `` tags. gh-aw safe outputs sanitize raw anchors before posting. +- Link to `visualAssets.commentUrl` when present, but do not embed or reproduce the + individual visual image URLs in the main report. The trusted publisher posts the + companion comment directly so its panels are not constrained by gh-aw safe-output + link limits. Visual publishing failures are limitations only; they do not weaken + or raise the gate. - Badge colors for the `Overall` (merge-readiness) badge: `1a7f37` for `Ready to merge` and `No failures found`, `d1242f` for `Not ready`, `bf8700` for `Needs human investigation`, and `6e7781` for `Insufficient data`. - Do not include a Data badge. - Do not use emojis anywhere in the posted comment. - Do not use `
` anywhere. Every collapsible section must be collapsed by default. -- Repeated `/review tests` runs post a new PR conversation comment and hide older comments from the same workflow. +- Repeated `/review tests` runs post a new PR conversation comment, hide older analysis + comments from the same workflow, and update the existing visual companion comment. - If there are no failing or inconclusive checks, still post the standard visible report with `Overall` = `No failures found`, `Failures` = `0`, no platform badges, and a recommendation that no test-failure action is needed. Use badge color `1a7f37`. diff --git a/.github/skills/review-test-failures/scripts/Gather-TestFailureContext.Tests.ps1 b/.github/skills/review-test-failures/scripts/Gather-TestFailureContext.Tests.ps1 index 4996e202c4b5..417cb5cbd4b2 100644 --- a/.github/skills/review-test-failures/scripts/Gather-TestFailureContext.Tests.ps1 +++ b/.github/skills/review-test-failures/scripts/Gather-TestFailureContext.Tests.ps1 @@ -36,7 +36,11 @@ BeforeAll { } foreach ($functionName in @( + 'ConvertTo-Array', 'Get-ObjectValue', + 'Get-VisualSnapshotInfo', + 'Select-VisualAttachments', + 'Get-VisualEnvironmentHintFromLog', 'Get-HelixWorkItemCounts', 'Get-XUnitFailures', 'Get-ConsoleFailureReason', @@ -57,6 +61,72 @@ BeforeAll { } } +Describe 'Visual snapshot evidence helpers' { + It 'parses a snapshot difference and its percentage' { + $info = Get-VisualSnapshotInfo -Message @' +VisualTestUtils.VisualTestFailedException : +Snapshot different than baseline: EntryClearButtonColorShouldUpdateOnThemeChange.png (2.08% difference) +If the correct baseline has changed, update it. +'@ + $info.kind | Should -Be 'different' + $info.snapshotFileName | Should -Be 'EntryClearButtonColorShouldUpdateOnThemeChange.png' + $info.description | Should -Be '2.08% difference' + $info.differencePercent | Should -Be 2.08 + } + + It 'parses a missing baseline and preserves a repository path hint' { + $info = Get-VisualSnapshotInfo -Message @' +Baseline snapshot not yet created: /agent/_work/1/s/src/Controls/tests/TestCases.iOS.Tests/snapshots/ios-26/NewSnapshot.png +Ensure new snapshot is correct. +'@ + $info.kind | Should -Be 'missing-baseline' + $info.snapshotFileName | Should -Be 'NewSnapshot.png' + $info.baselinePathHint | Should -Be 'src/Controls/tests/TestCases.iOS.Tests/snapshots/ios-26/NewSnapshot.png' + } + + It 'rejects an unsafe snapshot filename from untrusted test output' { + Get-VisualSnapshotInfo -Message 'Snapshot different than baseline: ../../payload.png (1.00% difference)' | Should -BeNullOrEmpty + } + + It 'selects the highest complete visual retry and ignores teardown screenshots' { + $attachments = @( + [pscustomobject]@{ id = 7; fileName = 'Sample-diff.png'; size = 10; url = 'https://dev.azure.com/o/p/_apis/test/Runs/1/Results/2/Attachments/7' }, + [pscustomobject]@{ id = 9; fileName = 'Sample-diff[1].png'; size = 10; url = 'https://dev.azure.com/o/p/_apis/test/Runs/1/Results/2/Attachments/9' }, + [pscustomobject]@{ id = 13; fileName = 'Sample-iOS-UITestBaseTearDown-ScreenShot-guid.png'; size = 10; url = 'https://dev.azure.com/o/p/_apis/test/Runs/1/Results/2/Attachments/13' }, + [pscustomobject]@{ id = 15; fileName = 'Sample.png'; size = 10; url = 'https://dev.azure.com/o/p/_apis/test/Runs/1/Results/2/Attachments/15' }, + [pscustomobject]@{ id = 17; fileName = 'Sample[1].png'; size = 10; url = 'https://dev.azure.com/o/p/_apis/test/Runs/1/Results/2/Attachments/17' } + ) + + $selected = Select-VisualAttachments -Attachments $attachments -SnapshotFileName 'Sample.png' + $selected.selectedRetry | Should -Be 1 + $selected.actual.id | Should -Be 17 + $selected.diff.id | Should -Be 9 + $selected.candidateCount | Should -Be 2 + } + + It 'keeps an actual-only attachment for a missing baseline' { + $selected = Select-VisualAttachments -Attachments @( + [pscustomobject]@{ id = 4; fileName = 'NewSnapshot.png'; size = 10; url = 'https://dev.azure.com/o/p/_apis/test/Runs/1/Results/2/Attachments/4' } + ) -SnapshotFileName 'NewSnapshot.png' + + $selected.actual.id | Should -Be 4 + $selected.diff | Should -BeNullOrEmpty + } + + It 'maps current UI runtime logs to snapshot environment directories' { + $ios = Get-VisualEnvironmentHintFromLog -Text 'Running TestCases.iOS.Tests --device="ios-simulator-64" --apiversion="26.0"' + $ios.platform | Should -Be 'ios' + $ios.environmentName | Should -Be 'ios-26' + + $android = Get-VisualEnvironmentHintFromLog -Text 'Running TestCases.Android.Tests --device="android-emulator-64" --apiversion="36"' + $android.platform | Should -Be 'android' + $android.environmentName | Should -Be 'android-notch-36' + + $mac = Get-VisualEnvironmentHintFromLog -Text 'Running TestCases.Mac.Tests for maccatalyst' + $mac.environmentName | Should -Be 'mac' + } +} + Describe 'Get-HelixWorkItemCounts (anonymous /workitems completeness + fail counting)' { It 'confirms a finished, full, all-pass job as NOT unverified, 0 failures' { $allPass = @(1..10 | ForEach-Object { [pscustomobject]@{ Name = "wi$_"; State = 'Finished'; ExitCode = 0 } }) diff --git a/.github/skills/review-test-failures/scripts/Gather-TestFailureContext.ps1 b/.github/skills/review-test-failures/scripts/Gather-TestFailureContext.ps1 index 3db016ec78b9..cc987399e6d9 100644 --- a/.github/skills/review-test-failures/scripts/Gather-TestFailureContext.ps1 +++ b/.github/skills/review-test-failures/scripts/Gather-TestFailureContext.ps1 @@ -244,6 +244,56 @@ function Get-AzDoTestRuns { return [ordered]@{ runs = $runs.ToArray(); truncated = $truncated } } +function Get-AzDoFailedTestResultsByBuild { + # The public vstmr endpoint exposes the failed-result identifiers that the ordinary + # _apis/test/runs list hides behind authentication. Those identifiers are enough to + # retrieve the public result detail and attachment metadata for visual failures. + param( + [string]$Org, + [string]$Project, + [int]$BuildId, + [int]$MaxPages = 10 + ) + + $results = New-Object System.Collections.Generic.List[object] + $continuation = $null + $truncated = $false + $page = 0 + do { + $page++ + $url = "https://vstmr.dev.azure.com/$Org/$Project/_apis/testresults/resultsbybuild?buildId=$BuildId&outcomes=Failed&`$top=10000&api-version=7.1-preview.1" + if ($continuation) { + $url += "&continuationToken=$([uri]::EscapeDataString([string]$continuation))" + } + + $response = Invoke-WebRequest -Uri $url -Headers @{ Accept = "application/json" } -UseBasicParsing -ErrorAction Stop + $body = if ([string]::IsNullOrWhiteSpace([string]$response.Content)) { + $null + } + else { + [string]$response.Content | ConvertFrom-Json + } + + foreach ($result in (ConvertTo-Array $body.value)) { + $results.Add($result) + } + + $continuation = Get-HeaderValue -Headers $response.Headers -Name 'x-ms-continuationtoken' + if ([string]::IsNullOrWhiteSpace($continuation)) { + $continuation = $null + } + if ($page -ge $MaxPages) { + $truncated = ($null -ne $continuation) + break + } + } while ($continuation) + + return [ordered]@{ + results = $results.ToArray() + truncated = $truncated + } +} + function Invoke-TextUrl { param( [Parameter(Mandatory = $true)] @@ -302,6 +352,225 @@ function Get-PlatformFromText { return "unknown" } +function Get-VisualSnapshotInfo { + param([string]$Message) + + if ([string]::IsNullOrWhiteSpace($Message)) { + return $null + } + + $different = [regex]::Match( + $Message, + '(?im)^\s*Snapshot different than baseline:\s*(?[^\r\n]*?\.png)\s*\((?[^\r\n)]*)\)') + $missing = [regex]::Match( + $Message, + '(?im)^\s*Baseline snapshot not yet created:\s*(?[^\r\n]*?\.png)\s*$') + + $kind = $null + $path = $null + $description = $null + if ($different.Success) { + $kind = "different" + $path = $different.Groups["path"].Value.Trim() + $description = $different.Groups["description"].Value.Trim() + } + elseif ($missing.Success) { + $kind = "missing-baseline" + $path = $missing.Groups["path"].Value.Trim() + } + else { + return $null + } + + $fileName = [System.IO.Path]::GetFileName($path) + if ([string]::IsNullOrWhiteSpace($fileName) -or + $fileName -notmatch '^[A-Za-z0-9][A-Za-z0-9._ -]*\.png$' -or + $fileName.Contains("..") -or + ($kind -eq "different" -and $path -ne $fileName)) { + return $null + } + + $differencePercent = $null + $baselineWidth = $null + $baselineHeight = $null + $actualWidth = $null + $actualHeight = $null + if ($description) { + $percentMatch = [regex]::Match($description, '^(?\d+(?:\.\d+)?)%\s+difference$') + if ($percentMatch.Success) { + $parsed = 0.0 + if ([double]::TryParse( + $percentMatch.Groups["value"].Value, + [System.Globalization.NumberStyles]::Float, + [System.Globalization.CultureInfo]::InvariantCulture, + [ref]$parsed)) { + $differencePercent = $parsed + } + } + + $sizeMatch = [regex]::Match( + $description, + '^size differs - baseline is (?\d+)x(?\d+) pixels, actual is (?\d+)x(?\d+) pixels$') + if ($sizeMatch.Success) { + $baselineWidth = [int]$sizeMatch.Groups["bw"].Value + $baselineHeight = [int]$sizeMatch.Groups["bh"].Value + $actualWidth = [int]$sizeMatch.Groups["aw"].Value + $actualHeight = [int]$sizeMatch.Groups["ah"].Value + } + } + + $pathHint = $null + $normalizedPath = $path -replace '\\', '/' + $snapshotPathMatch = [regex]::Match( + $normalizedPath, + '(?i)(?src/Controls/tests/TestCases\.[^/]+\.Tests/snapshots/[^/]+/[^/]+\.png)$') + if ($snapshotPathMatch.Success) { + $pathHint = $snapshotPathMatch.Groups["path"].Value + } + + return [ordered]@{ + kind = $kind + snapshotFileName = $fileName + description = $description + differencePercent = $differencePercent + baselineWidth = $baselineWidth + baselineHeight = $baselineHeight + actualWidth = $actualWidth + actualHeight = $actualHeight + baselinePathHint = $pathHint + } +} + +function Select-VisualAttachments { + param( + [object[]]$Attachments, + [string]$SnapshotFileName + ) + + if ([string]::IsNullOrWhiteSpace($SnapshotFileName)) { + return [ordered]@{ actual = $null; diff = $null; selectedRetry = $null; candidateCount = 0 } + } + + $stem = [System.IO.Path]::GetFileNameWithoutExtension($SnapshotFileName) + $escapedStem = [regex]::Escape($stem) + $actualByRetry = @{} + $diffByRetry = @{} + + foreach ($attachment in (ConvertTo-Array $Attachments)) { + $fileName = [string](Get-ObjectValue -Object $attachment -Names @("fileName", "name")) + $id = Get-ObjectValue -Object $attachment -Names @("id") + $url = [string](Get-ObjectValue -Object $attachment -Names @("url")) + if ([string]::IsNullOrWhiteSpace($fileName) -or + [string]::IsNullOrWhiteSpace($url) -or + $null -eq $id) { + continue + } + + $actualMatch = [regex]::Match($fileName, "(?i)^$escapedStem(?:\[(?\d+)\])?\.png$") + $diffMatch = [regex]::Match($fileName, "(?i)^$escapedStem-diff(?:\[(?\d+)\])?\.png$") + if (-not $actualMatch.Success -and -not $diffMatch.Success) { + continue + } + + $match = if ($actualMatch.Success) { $actualMatch } else { $diffMatch } + $retry = if ($match.Groups["retry"].Success) { [int]$match.Groups["retry"].Value } else { 0 } + $metadata = [ordered]@{ + id = [int]$id + fileName = $fileName + size = Get-ObjectValue -Object $attachment -Names @("size") + url = $url + } + if ($actualMatch.Success) { + $actualByRetry[$retry] = $metadata + } + else { + $diffByRetry[$retry] = $metadata + } + } + + $allRetries = @($actualByRetry.Keys + $diffByRetry.Keys | Sort-Object -Unique -Descending) + $selectedRetry = $null + foreach ($retry in $allRetries) { + if ($actualByRetry.ContainsKey($retry) -and $diffByRetry.ContainsKey($retry)) { + $selectedRetry = [int]$retry + break + } + } + if ($null -eq $selectedRetry -and $actualByRetry.Count -gt 0) { + $selectedRetry = [int](@($actualByRetry.Keys | Sort-Object -Descending)[0]) + } + + return [ordered]@{ + actual = $(if ($null -ne $selectedRetry -and $actualByRetry.ContainsKey($selectedRetry)) { $actualByRetry[$selectedRetry] } else { $null }) + diff = $(if ($null -ne $selectedRetry -and $diffByRetry.ContainsKey($selectedRetry)) { $diffByRetry[$selectedRetry] } else { $null }) + selectedRetry = $selectedRetry + candidateCount = $allRetries.Count + } +} + +function Get-VisualEnvironmentHintFromLog { + param([string]$Text) + + if ([string]::IsNullOrWhiteSpace($Text)) { + return $null + } + + $platform = if ($Text -match '(?i)TestCases\.iOS\.Tests|ios-simulator') { + "ios" + } + elseif ($Text -match '(?i)TestCases\.Android\.Tests|android-emulator') { + "android" + } + elseif ($Text -match '(?i)TestCases\.Mac\.Tests|maccatalyst') { + "macos" + } + elseif ($Text -match '(?i)TestCases\.WinUI\.Tests|winui_ui_tests') { + "windows" + } + else { + return $null + } + + $environmentName = $null + $version = $null + if ($platform -eq "ios" -or $platform -eq "android") { + $versionMatch = [regex]::Match($Text, '(?im)--apiversion(?:=|\s+)["'']*(?\d+(?:\.\d+)*)') + if ($versionMatch.Success) { + $version = $versionMatch.Groups["version"].Value + } + } + + switch ($platform) { + "ios" { + if ($version -match '^26(?:\.|$)') { + $environmentName = "ios-26" + } + elseif ($Text -match '(?i)iPhone X \(iOS 16\.4\)') { + $environmentName = "ios-iphonex" + } + else { + $environmentName = "ios" + } + } + "android" { + if ($version -match '^36(?:\.|$)') { + $environmentName = "android-notch-36" + } + else { + $environmentName = "android" + } + } + "macos" { $environmentName = "mac" } + "windows" { $environmentName = "windows" } + } + + return [ordered]@{ + platform = $platform + environmentName = $environmentName + apiVersion = $version + } +} + function Get-AreaHintsFromPath { param([string]$Path) @@ -2016,6 +2285,8 @@ foreach ($ref in $manualBuildRefs.ToArray()) { $builds = New-Object System.Collections.Generic.List[object] $allLogFailures = New-Object System.Collections.Generic.List[object] $allLogExcerpts = New-Object System.Collections.Generic.List[object] +$allVisualEvidence = New-Object System.Collections.Generic.List[object] +$visualEvidenceLimitations = New-Object System.Collections.Generic.List[string] # Failed Task legs whose log was read but yielded NO extractable failure (test OR build # error). This is the backstop for the "never wrong again" guarantee: even if a novel # break shape escapes both extractors, a failed-but-unexplained leg forces the verdict @@ -2048,6 +2319,8 @@ foreach ($buildRef in $buildRefsById.Values) { logExcerpts = @() testFailuresFromLogs = @() testResults = @() + visualEnvironmentHints = @() + visualEvidence = @() helix = [ordered]@{ checked = $false jobIds = @() @@ -2152,6 +2425,17 @@ foreach ($buildRef in $buildRefsById.Values) { $logText = Invoke-TextUrl -Url "$baseUrl/_apis/build/builds/$($buildRef.buildId)/logs/$logId`?api-version=7.1" $lines = @($logText -split "`r?`n") + $visualEnvironmentHint = Get-VisualEnvironmentHintFromLog -Text $logText + if ($visualEnvironmentHint) { + $existingHint = @($buildSummary.visualEnvironmentHints | Where-Object { + $_.platform -eq $visualEnvironmentHint.platform -and + $_.environmentName -eq $visualEnvironmentHint.environmentName + }) + if ($existingHint.Count -eq 0) { + $buildSummary.visualEnvironmentHints += @($visualEnvironmentHint) + } + } + $excerpts = @(Get-LogExcerpts -Lines $lines -LogId $logId -RecordName $record.name) foreach ($excerpt in $excerpts) { $allLogExcerpts.Add($excerpt) @@ -2590,6 +2874,107 @@ foreach ($buildRef in $buildRefsById.Values) { } } + if ($build.definition.name -eq "maui-pr-uitests") { + try { + $failedResultPage = Get-AzDoFailedTestResultsByBuild ` + -Org $buildRef.org ` + -Project $buildRef.project ` + -BuildId $buildRef.buildId + $failedResultsAll = @($failedResultPage.results) + $failedResults = @($failedResultsAll | Select-Object -First 100) + + if ($failedResultPage.truncated) { + $visualEvidenceLimitations.Add("Visual result discovery for AzDO build $($buildRef.buildId) stopped at the pagination guard; some screenshot comparisons may be omitted.") + } + if ($failedResultsAll.Count -gt $failedResults.Count) { + $visualEvidenceLimitations.Add("Visual result discovery for AzDO build $($buildRef.buildId) inspected the first $($failedResults.Count) of $($failedResultsAll.Count) failed test results.") + } + + foreach ($failedResult in $failedResults) { + $runId = [int](Get-ObjectValue -Object $failedResult -Names @("runId")) + $resultId = [int](Get-ObjectValue -Object $failedResult -Names @("id")) + if ($runId -le 0 -or $resultId -le 0) { + continue + } + + try { + $resultUrl = "$baseUrl/_apis/test/Runs/$runId/Results/$resultId`?detailsToInclude=Iterations&api-version=7.1" + $detail = Invoke-JsonUrl -Url $resultUrl -AllowAuth + $message = [string](Get-ObjectValue -Object $detail -Names @("errorMessage")) + $snapshotInfo = Get-VisualSnapshotInfo -Message $message + if (-not $snapshotInfo) { + continue + } + + $attachmentsUrl = "$baseUrl/_apis/test/Runs/$runId/Results/$resultId/attachments?api-version=7.1" + $attachmentResponse = Invoke-JsonUrl -Url $attachmentsUrl -AllowAuth + $selectedAttachments = Select-VisualAttachments ` + -Attachments (ConvertTo-Array $attachmentResponse.value) ` + -SnapshotFileName $snapshotInfo.snapshotFileName + + if (-not $selectedAttachments.actual) { + $visualEvidenceLimitations.Add("Visual result $runId/$resultId in AzDO build $($buildRef.buildId) named '$($snapshotInfo.snapshotFileName)' but exposed no matching actual-image attachment.") + continue + } + + $testName = [string](Get-ObjectValue -Object $detail -Names @("testCaseTitle") -Default ( + Get-ObjectValue -Object $detail.testCase -Names @("name") -Default ( + Get-ObjectValue -Object $failedResult -Names @("testCaseTitle") -Default $snapshotInfo.snapshotFileName + ) + )) + $automatedTestName = [string](Get-ObjectValue -Object $detail -Names @("automatedTestName") -Default ( + Get-ObjectValue -Object $failedResult -Names @("automatedTestName") + )) + $runName = [string](Get-ObjectValue -Object $detail.testRun -Names @("name")) + $platform = Get-PlatformFromText -Text "$runName $automatedTestName $($detail.automatedTestStorage)" + $environmentHints = @($buildSummary.visualEnvironmentHints | Where-Object { $_.platform -eq $platform }) + $environmentNames = @($environmentHints | ForEach-Object { $_.environmentName } | Where-Object { $_ } | Select-Object -Unique) + $environmentName = if ($environmentNames.Count -eq 1) { $environmentNames[0] } else { $null } + + $evidence = [ordered]@{ + testName = $testName + automatedTestName = $automatedTestName + platform = $platform + buildId = $buildRef.buildId + buildDefinition = $build.definition.name + buildUrl = $build._links.web.href + buildSourceVersion = $build.sourceVersion + runId = $runId + runName = $runName + resultId = $resultId + completedDate = $detail.completedDate + resultUrl = $resultUrl + message = $message + kind = $snapshotInfo.kind + snapshotFileName = $snapshotInfo.snapshotFileName + description = $snapshotInfo.description + differencePercent = $snapshotInfo.differencePercent + baselineWidth = $snapshotInfo.baselineWidth + baselineHeight = $snapshotInfo.baselineHeight + actualWidth = $snapshotInfo.actualWidth + actualHeight = $snapshotInfo.actualHeight + baselinePathHint = $snapshotInfo.baselinePathHint + environmentName = $environmentName + environmentHints = $environmentHints + attachmentsListUrl = $attachmentsUrl + selectedRetry = $selectedAttachments.selectedRetry + actual = $selectedAttachments.actual + diff = $selectedAttachments.diff + } + + $buildSummary.visualEvidence += @($evidence) + $allVisualEvidence.Add($evidence) + } + catch { + $visualEvidenceLimitations.Add("Visual result detail $runId/$resultId in AzDO build $($buildRef.buildId) could not be inspected: $($_.Exception.Message)") + } + } + } + catch { + $visualEvidenceLimitations.Add("Visual result discovery failed for AzDO build $($buildRef.buildId): $($_.Exception.Message)") + } + } + if (-not [string]::IsNullOrWhiteSpace($env:AZDO_TOKEN)) { try { # Page through ALL test runs. The endpoint returns only one ~100-run page per call; summing @@ -2715,6 +3100,7 @@ foreach ($buildRef in $buildRefsById.Values) { $allFailuresArray = $allLogFailures.ToArray() $allExcerptsArray = $allLogExcerpts.ToArray() +$visualEvidenceArray = $allVisualEvidence.ToArray() $buildArray = $builds.ToArray() $dedupedFailures = @(Get-DeduplicatedFailures -Failures $allFailuresArray) @@ -3519,13 +3905,13 @@ if ($ciScanIssues.error) { } $context = [ordered]@{ - schemaVersion = 1 + schemaVersion = 2 generatedAtUtc = (Get-Date).ToUniversalTime().ToString("o") repository = $Repository azdo = [ordered]@{ authenticated = -not [string]::IsNullOrWhiteSpace($env:AZDO_TOKEN) authSource = $script:AzDoAuthSource - dataSourceGuidance = "Uses AzDO build, timeline, and build log REST APIs as the primary data source; authenticated _apis/test queries are optional and only attempted when an AzDO bearer token is available." + dataSourceGuidance = "Uses AzDO build, timeline, and build log REST APIs as the primary data source; public vstmr failed-result metadata is used for UI visual evidence; authenticated _apis/test queries remain optional." } pr = [ordered]@{ number = $pr.number @@ -3566,6 +3952,11 @@ $context = [ordered]@{ } buildRefs = @($buildRefsById.Values) builds = $buildArray + visualEvidence = [ordered]@{ + detected = $visualEvidenceArray.Count + comparisons = $visualEvidenceArray + limitations = $visualEvidenceLimitations.ToArray() + } failures = [ordered]@{ unique = $dedupedFailures baseline = $baselineDeduped @@ -3624,6 +4015,23 @@ $md.Add("- Inferred platforms from files: $(@($inferredPlatforms) -join ', ')") $md.Add("- Area labels: $(@($areaLabels) -join ', ')") $md.Add("- Area hints from files: $(@($areaHints) -join ', ')") $md.Add("") +$md.Add("## Visual snapshot evidence") +$md.Add("") +$md.Add("- Visual comparisons detected: $($visualEvidenceArray.Count)") +if ($visualEvidenceArray.Count -gt 0) { + foreach ($visual in $visualEvidenceArray) { + $description = if ($visual.description) { $visual.description } else { $visual.kind } + $environment = if ($visual.environmentName) { " · baseline environment $($visual.environmentName)" } else { "" } + $md.Add(" - $($visual.snapshotFileName) on $($visual.platform) (build $($visual.buildId), run $($visual.runId), result $($visual.resultId)): $description$environment") + } +} +if ($visualEvidenceLimitations.Count -gt 0) { + $md.Add("- Visual evidence limitations:") + foreach ($visualLimitation in $visualEvidenceLimitations) { + $md.Add(" - $visualLimitation") + } +} +$md.Add("") $md.Add("## Interesting checks") $md.Add("") if ($interestingChecks.Count -eq 0) { diff --git a/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 b/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 new file mode 100644 index 000000000000..a186a4c1b758 --- /dev/null +++ b/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 @@ -0,0 +1,251 @@ +#!/usr/bin/env pwsh +#Requires -Modules Pester + +BeforeAll { + $scriptPath = Join-Path $PSScriptRoot 'Publish-TestVisualAssets.ps1' + $tokens = $null + $parseErrors = $null + $ast = [System.Management.Automation.Language.Parser]::ParseFile($scriptPath, [ref]$tokens, [ref]$parseErrors) + if ($parseErrors -and $parseErrors.Count -gt 0) { + throw ($parseErrors | ForEach-Object { $_.Message }) -join [Environment]::NewLine + } + + foreach ($functionName in @( + 'Escape-Html', + 'Get-SafeAssetSlug', + 'Invoke-GhApiJson', + 'Test-PngFile', + 'Test-AzDoAttachmentUrl', + 'Get-SnapshotRoot', + 'Get-SnapshotCandidatePaths', + 'New-VisualComparisonsMarkdown', + 'Get-ExistingVisualComments', + 'New-VisualCommentBody', + 'Publish-VisualComment' + )) { + $function = $ast.Find({ + $args[0] -is [System.Management.Automation.Language.FunctionDefinitionAst] -and + $args[0].Name -eq $functionName + }, $true) + if (-not $function) { throw "Function '$functionName' not found in $scriptPath" } + Invoke-Expression $function.Extent.Text + } +} + +Describe 'Visual asset input validation' { + It 'accepts only the exact public AzDO attachment URL for the expected result' { + Test-AzDoAttachmentUrl ` + -Url 'https://dev.azure.com/dnceng-public/public/_apis/test/Runs/12/Results/34/Attachments/56' ` + -RunId 12 ` + -ResultId 34 ` + -AttachmentId 56 | Should -BeTrue + + Test-AzDoAttachmentUrl ` + -Url 'https://evil.example/dnceng-public/public/_apis/test/Runs/12/Results/34/Attachments/56' ` + -RunId 12 ` + -ResultId 34 ` + -AttachmentId 56 | Should -BeFalse + + Test-AzDoAttachmentUrl ` + -Url 'https://dev.azure.com/dnceng-public/public/_apis/test/Runs/12/Results/99/Attachments/56' ` + -RunId 12 ` + -ResultId 34 ` + -AttachmentId 56 | Should -BeFalse + } + + It 'validates the PNG signature and size bound without decoding untrusted image data' { + $valid = Join-Path $TestDrive 'valid.png' + [System.IO.File]::WriteAllBytes( + $valid, + [Convert]::FromBase64String('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M/wHwAF/gL+XyO8WQAAAABJRU5ErkJggg==')) + Test-PngFile -Path $valid -MaximumBytes 1024 | Should -BeTrue + + $invalid = Join-Path $TestDrive 'invalid.png' + [System.IO.File]::WriteAllText($invalid, 'not an image') + Test-PngFile -Path $invalid -MaximumBytes 1024 | Should -BeFalse + Test-PngFile -Path $valid -MaximumBytes 8 | Should -BeFalse + + $oversizedDimensions = Join-Path $TestDrive 'oversized-dimensions.png' + $bytes = [System.IO.File]::ReadAllBytes($valid) + $bytes[16] = 0x00 + $bytes[17] = 0x01 + $bytes[18] = 0x00 + $bytes[19] = 0x01 + [System.IO.File]::WriteAllBytes($oversizedDimensions, $bytes) + Test-PngFile -Path $oversizedDimensions -MaximumBytes 1024 | Should -BeFalse + } + + It 'normalizes untrusted names into bounded asset slugs' { + Get-SafeAssetSlug -Value '../My Name?!' | Should -Be 'my-snapshot-name' + (Get-SafeAssetSlug -Value ('A' * 200)).Length | Should -BeLessOrEqual 72 + } +} + +Describe 'Snapshot baseline candidates' { + It 'puts the runtime environment and trusted path hint before fallback directories' { + $root = Join-Path $TestDrive 'repo' + $snapshotRoot = Join-Path $root 'src/Controls/tests/TestCases.iOS.Tests/snapshots' + New-Item -ItemType Directory -Force -Path (Join-Path $snapshotRoot 'ios') | Out-Null + New-Item -ItemType Directory -Force -Path (Join-Path $snapshotRoot 'ios-26') | Out-Null + + $paths = @(Get-SnapshotCandidatePaths ` + -Platform 'ios' ` + -SnapshotFileName 'Sample.png' ` + -EnvironmentName 'ios-26' ` + -BaselinePathHint 'src/Controls/tests/TestCases.iOS.Tests/snapshots/ios-26/Sample.png' ` + -RepositoryRoot $root) + + $paths[0] | Should -Be 'src/Controls/tests/TestCases.iOS.Tests/snapshots/ios-26/Sample.png' + $paths | Should -Contain 'src/Controls/tests/TestCases.iOS.Tests/snapshots/ios/Sample.png' + $paths.Count | Should -Be 2 + } + + It 'rejects unsafe filenames and path hints' { + @(Get-SnapshotCandidatePaths ` + -Platform 'ios' ` + -SnapshotFileName '../payload.png' ` + -EnvironmentName 'ios' ` + -BaselinePathHint '../../payload.png' ` + -RepositoryRoot $TestDrive).Count | Should -Be 0 + } +} + +Describe 'Visual comparison markdown' { + It 'renders escaped, collapsed three-column comparison panels' { + $markdown = New-VisualComparisonsMarkdown -Comparisons @( + [pscustomobject]@{ + testName = '' + platform = 'iOS' + description = '2.08% difference' + buildId = 123 + buildUrl = 'https://dev.azure.com/example' + baselineUrl = 'https://raw.githubusercontent.com/org/repo/sha/base.png' + baselineStatus = 'resolved' + actualUrl = 'https://raw.githubusercontent.com/org/repo/sha/actual.png' + diffUrl = 'https://raw.githubusercontent.com/org/repo/sha/diff.png' + } + ) -OmittedCount 0 -MaximumCharacters 5000 + + $markdown | Should -Match '
' + $markdown | Should -Not -Match '
CI baselineFresh PR actualCI diff' + $markdown | Should -Match '<script>alert\(1\)</script>' + $markdown | Should -Not -Match '' + + $escaped | Should -Be '<script>@everyone & "quoted"</script>' + [regex]::Matches($escaped, '@\w+').Count | Should -Be 0 + } + + It 'uses the same URL, mention, and UTF-16 character counting shape as gh-aw' { + $body = 'https://one.example/a https://two.example/b > @author email@test' + $counts = Get-CommentLimitCounts -Body $body + + $counts.urls | Should -Be 2 + $counts.mentions | Should -Be 2 + $counts.characters | Should -Be $body.Length + } +} + +Describe 'Inline visual body merge' { + It 'replaces the trusted placeholder with escaped expandable panels inside one comment' { + $comparison = New-VisualTestComparison -TestName '@danger' + $context = New-VisualTestContext -Comparisons @($comparison) + $body = @' +## Tests Failure Analysis + +> @author - results + +
+Test Failure Review + +**Overall verdict:** Not ready. + + + +### Recommended action + +Investigate. +
+'@ + + $merged = Merge-VisualsIntoBody ` + -Body $body ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + + $merged | Should -Match ([regex]::Escape((Get-InlineVisualStartMarker))) + $merged | Should -Not -Match ([regex]::Escape((Get-InlineVisualPlaceholder))) + $merged | Should -Match '<b>@danger</b>' + $merged | Should -Match 'CI baselineFresh PR actualCI diff' + $merged.IndexOf('### Visual failure comparisons') | + Should -BeLessThan $merged.IndexOf('### Recommended action') + ([regex]::Matches($merged, '
').Count) | Should -Be 2 + + $counts = Get-CommentLimitCounts -Body $merged + $counts.urls | Should -Be 3 + $counts.mentions | Should -Be 1 + $counts.characters | Should -BeLessOrEqual 60000 + } + + It 'dynamically omits panels that would exceed the final comment URL budget' { + $existingUrls = (1..43 | ForEach-Object { "https://example.com/$_" }) -join ' ' + $body = "$existingUrls`n
`n`n
" + $context = New-VisualTestContext + + $merged = Merge-VisualsIntoBody ` + -Body $body ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + + ([regex]::Matches($merged, 'visual comparison').Count) | Should -Be 0 + $merged | Should -Match '1 additional comparison\(s\) were omitted' + (Get-CommentLimitCounts -Body $merged).urls | Should -Be 43 + } + + It 'can still fit a later one-image panel after a three-image panel is omitted' { + $existingUrls = (1..43 | ForEach-Object { "https://example.com/$_" }) -join ' ' + $comparisons = @( + (New-VisualTestComparison -TestName 'ThreeImages'), + (New-VisualTestComparison -TestName 'ActualOnly' -ActualOnly) + ) + $context = New-VisualTestContext -Comparisons $comparisons + $body = "$existingUrls`n
`n`n
" + + $merged = Merge-VisualsIntoBody ` + -Body $body ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + + $merged | Should -Not -Match 'ThreeImages' + $merged | Should -Match 'ActualOnly' + (Get-CommentLimitCounts -Body $merged).urls | Should -Be 44 + } + + It 'bounds by final character count without truncating HTML mid-panel' { + $comparison = New-VisualTestComparison -TestName ('LongName' + ('x' * 500)) + $context = New-VisualTestContext -Comparisons @($comparison) + $body = ('a' * 1200) + "`n
`n`n
" + + $merged = Merge-VisualsIntoBody ` + -Body $body ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 1700 + + $merged.Length | Should -BeLessOrEqual 1700 + ([regex]::Matches($merged, '
').Count) | + Should -Be ([regex]::Matches($merged, '
').Count) + $merged | Should -Match 'additional comparison\(s\) were omitted' + } + + It 'removes the placeholder without adding a section when no assets were published' { + $context = New-VisualTestContext -Published $false + $body = "Analysis`n`nDone" + + $merged = Merge-VisualsIntoBody ` + -Body $body ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + + $merged | Should -Be "Analysis`n`nDone" + } + + It 'skips an invalid actual URL and reports the comparison as omitted' { + $comparison = New-VisualTestComparison + $comparison.actualUrl = 'https://evil.example/payload.png' + $context = New-VisualTestContext -Comparisons @($comparison) + + $merged = Merge-VisualsIntoBody ` + -Body '
' ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + + $merged | Should -Not -Match 'evil\.example' + $merged | Should -Match '1 additional comparison\(s\) were omitted' + } + + It 'is idempotent within the pre-post payload' { + $context = New-VisualTestContext + $body = '
' + + $first = Merge-VisualsIntoBody ` + -Body $body ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + $second = Merge-VisualsIntoBody ` + -Body $first ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + + ([regex]::Matches($second, [regex]::Escape((Get-InlineVisualStartMarker))).Count) | + Should -Be 1 + ([regex]::Matches($second, 'visual comparison').Count) | Should -Be 1 + } + + It 'appends the section when an agent omits both the placeholder and details wrapper' { + $context = New-VisualTestContext + $merged = Merge-VisualsIntoBody ` + -Body 'Short failure report.' ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + + $merged | Should -Match '^Short failure report\.' + $merged | Should -Match '### Visual failure comparisons' + } +} + +Describe 'Agent output mutation' { + It 'atomically updates only the matching add_comment item and preserves its schema' { + $context = New-VisualTestContext + $path = Join-Path $TestDrive 'agent_output.json' + @{ + errors = @() + items = @( + @{ + type = 'add_comment' + item_number = 123 + body = '
' + temporary_id = 'aw_123' + }, + @{ + type = 'noop' + message = 'keep me' + } + ) + } | ConvertTo-Json -Depth 10 | Set-Content -LiteralPath $path -Encoding UTF8 + + $result = Update-AgentOutputFile ` + -Path $path ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + + $result.changed | Should -BeTrue + $result.mergedComments | Should -Be 1 + $updated = Get-Content -LiteralPath $path -Raw -Encoding UTF8 | ConvertFrom-Json + @($updated.items).Count | Should -Be 2 + $updated.items[0].temporary_id | Should -Be 'aw_123' + $updated.items[0].body | Should -Match '### Visual failure comparisons' + $updated.items[1].message | Should -Be 'keep me' + } + + It 'leaves noop-only output byte-for-byte unchanged' { + $context = New-VisualTestContext + $path = Join-Path $TestDrive 'noop_output.json' + $original = '{"errors":[],"items":[{"type":"noop","message":"dry run"}]}' + [System.IO.File]::WriteAllText($path, $original) + + $result = Update-AgentOutputFile ` + -Path $path ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + + $result.changed | Should -BeFalse + [System.IO.File]::ReadAllText($path) | Should -BeExactly $original + } + + It 'does not damage malformed agent output when parsing fails' { + $context = New-VisualTestContext + $path = Join-Path $TestDrive 'malformed.json' + $original = '{"items":[' + [System.IO.File]::WriteAllText($path, $original) + + { + Update-AgentOutputFile ` + -Path $path ` + -Context $context ` + -Repository 'dotnet/maui' ` + -PrNumber 123 ` + -MaxCommentUrls 45 ` + -MaxCommentMentions 10 ` + -MaxCommentCharacters 60000 + } | Should -Throw + [System.IO.File]::ReadAllText($path) | Should -BeExactly $original + } +} diff --git a/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 b/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 new file mode 100644 index 000000000000..00ecd307ae65 --- /dev/null +++ b/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 @@ -0,0 +1,571 @@ +#!/usr/bin/env pwsh +<# +.SYNOPSIS + Merges trusted visual comparisons into the single /review tests comment. + +.DESCRIPTION + Reads visual asset metadata produced by Publish-TestVisualAssets.ps1 and inserts + bounded, expandable comparison panels into an existing analysis comment. The + merger enforces gh-aw's final comment limits before changing any output. + + In AgentOutput mode, the script atomically updates the add_comment item in + /tmp/gh-aw/agent_output.json. In CommentBody mode, it atomically updates a local + Markdown comment file. +#> + +[CmdletBinding(DefaultParameterSetName = "AgentOutput")] +param( + [Parameter(Mandatory = $true)] + [int]$PrNumber, + + [Parameter(Mandatory = $true)] + [string]$ContextJsonPath, + + [Parameter(Mandatory = $false)] + [string]$Repository = $env:GITHUB_REPOSITORY, + + [Parameter(Mandatory = $true, ParameterSetName = "AgentOutput")] + [string]$AgentOutputPath, + + [Parameter(Mandatory = $true, ParameterSetName = "CommentBody")] + [string]$CommentBodyPath, + + [Parameter(Mandatory = $false)] + [ValidateRange(1, 49)] + [int]$MaxCommentUrls = 45, + + [Parameter(Mandatory = $false)] + [ValidateRange(1, 10)] + [int]$MaxCommentMentions = 10, + + [Parameter(Mandatory = $false)] + [ValidateRange(1000, 65000)] + [int]$MaxCommentCharacters = 60000 +) + +$ErrorActionPreference = "Stop" + +function Get-InlineVisualStartMarker { + return "" +} + +function Get-InlineVisualEndMarker { + return "" +} + +function Get-InlineVisualPlaceholder { + return "" +} + +function Get-BoundedText { + param( + [string]$Value, + [int]$MaximumLength + ) + + if ([string]::IsNullOrEmpty($Value) -or $Value.Length -le $MaximumLength) { + return $Value + } + if ($MaximumLength -le 3) { + return $Value.Substring(0, $MaximumLength) + } + return $Value.Substring(0, $MaximumLength - 3) + "..." +} + +function Escape-VisualText { + param( + [string]$Value, + [int]$MaximumLength = 240 + ) + + if ($null -eq $Value) { + return "" + } + + $bounded = Get-BoundedText -Value $Value -MaximumLength $MaximumLength + return [System.Net.WebUtility]::HtmlEncode($bounded).Replace("@", "@") +} + +function Test-VisualAssetUrl { + param( + [string]$Url, + [string]$Repository, + [int]$PrNumber, + [string]$AssetCommit + ) + + if ([string]::IsNullOrWhiteSpace($Url) -or $Url.Length -gt 2048) { + return $false + } + if ($Repository -notmatch '^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$' -or + $AssetCommit -notmatch '^[0-9a-fA-F]{40}$') { + return $false + } + + $uri = $null + if (-not [Uri]::TryCreate($Url, [UriKind]::Absolute, [ref]$uri)) { + return $false + } + if ($uri.Scheme -ne "https" -or + $uri.Host -ne "raw.githubusercontent.com" -or + -not $uri.IsDefaultPort -or + $uri.UserInfo -or + $uri.Query -or + $uri.Fragment) { + return $false + } + + try { + $path = [Uri]::UnescapeDataString($uri.AbsolutePath) + } + catch { + return $false + } + + $repositoryParts = $Repository.Split("/") + $expectedPrefix = "/$($repositoryParts[0])/$($repositoryParts[1])/$AssetCommit/pr-$PrNumber/" + if (-not $path.StartsWith($expectedPrefix, [StringComparison]::OrdinalIgnoreCase)) { + return $false + } + + $relativePath = $path.Substring($expectedPrefix.Length) + if ([string]::IsNullOrWhiteSpace($relativePath) -or + $relativePath.Contains("..") -or + $relativePath.Contains("//") -or + $relativePath.Contains("\") -or + $relativePath -notmatch '^[A-Za-z0-9._/-]+\.png$') { + return $false + } + + return @($relativePath.Split("/") | Where-Object { $_ -eq "." -or $_ -eq ".." }).Count -eq 0 +} + +function Get-CommentLimitCounts { + param([string]$Body) + + if ($null -eq $Body) { + $Body = "" + } + + return [pscustomobject]@{ + urls = [regex]::Matches($Body, 'https?://[^\s]+').Count + mentions = [regex]::Matches($Body, '@\w+').Count + characters = $Body.Length + } +} + +function Remove-InlineVisualSection { + param([string]$Body) + + if ([string]::IsNullOrEmpty($Body)) { + return $Body + } + + $pattern = [regex]::Escape((Get-InlineVisualStartMarker)) + + '.*?' + + [regex]::Escape((Get-InlineVisualEndMarker)) + return [regex]::Replace( + $Body, + $pattern, + "", + [System.Text.RegularExpressions.RegexOptions]::Singleline, + [TimeSpan]::FromSeconds(1)) +} + +function New-InlineVisualPanel { + param( + [object]$Comparison, + [string]$BaselineUrl, + [string]$ActualUrl, + [string]$DiffUrl + ) + + $testName = Escape-VisualText -Value ([string]$Comparison.testName) -MaximumLength 180 + $platform = Escape-VisualText -Value ([string]$Comparison.platform) -MaximumLength 40 + $description = Escape-VisualText -Value ([string]$Comparison.description) -MaximumLength 240 + $baselineStatus = Escape-VisualText -Value ([string]$Comparison.baselineStatus) -MaximumLength 180 + $baselineAlt = Escape-VisualText -Value "$([string]$Comparison.testName) baseline" -MaximumLength 220 + $actualAlt = Escape-VisualText -Value "$([string]$Comparison.testName) actual" -MaximumLength 220 + $diffAlt = Escape-VisualText -Value "$([string]$Comparison.testName) diff" -MaximumLength 220 + $safeActualUrl = Escape-VisualText -Value $ActualUrl -MaximumLength 2048 + $buildId = [int]$Comparison.buildId + + $baselineCell = if ($BaselineUrl) { + $safeBaselineUrl = Escape-VisualText -Value $BaselineUrl -MaximumLength 2048 + "" + } + else { + "Baseline unavailable: $baselineStatus" + } + $diffCell = if ($DiffUrl) { + $safeDiffUrl = Escape-VisualText -Value $DiffUrl -MaximumLength 2048 + "" + } + else { + "CI diff was not generated." + } + $descriptionLine = if ($description) { + "CI reported $description in build $buildId." + } + else { + "CI reported a visual snapshot failure in build $buildId." + } + + return @" +
+$testName - $platform - visual comparison + +$descriptionLine + + + + + +
CI baselineFresh PR actualCI diff
$baselineCell$actualAlt$diffCell
+
+ +"@ +} + +function New-InlineVisualSection { + param( + [string[]]$Panels, + [int]$OmittedCount + ) + + $builder = [System.Text.StringBuilder]::new() + [void]$builder.AppendLine((Get-InlineVisualStartMarker)) + [void]$builder.AppendLine("### Visual failure comparisons") + [void]$builder.AppendLine() + [void]$builder.AppendLine("Full-resolution CI baseline, actual, and diff images are embedded below. They supplement the failure classification and do not change the deterministic verdict ceiling.") + [void]$builder.AppendLine() + + foreach ($panel in @($Panels)) { + [void]$builder.Append($panel) + } + + if (@($Panels).Count -eq 0 -and $OmittedCount -gt 0) { + [void]$builder.AppendLine("Visual comparisons were detected, but none fit within the comment safety limits.") + [void]$builder.AppendLine() + } + if ($OmittedCount -gt 0) { + [void]$builder.AppendLine("Visual output was bounded for comment safety; $OmittedCount additional comparison(s) were omitted.") + } + + [void]$builder.AppendLine((Get-InlineVisualEndMarker)) + return $builder.ToString() +} + +function Insert-InlineVisualSection { + param( + [string]$Body, + [string]$Section + ) + + $placeholder = Get-InlineVisualPlaceholder + $placeholderIndex = $Body.IndexOf($placeholder, [StringComparison]::Ordinal) + if ($placeholderIndex -ge 0) { + $prefix = $Body.Substring(0, $placeholderIndex) + $suffix = $Body.Substring($placeholderIndex + $placeholder.Length).Replace($placeholder, "") + return $prefix + $Section + $suffix + } + + $withoutPlaceholders = $Body.Replace($placeholder, "") + $closingDetailsIndex = $withoutPlaceholders.LastIndexOf("
", [StringComparison]::OrdinalIgnoreCase) + if ($closingDetailsIndex -ge 0) { + return $withoutPlaceholders.Insert($closingDetailsIndex, "$Section`n") + } + + return $withoutPlaceholders.TrimEnd() + "`n`n" + $Section +} + +function Test-CommentWithinLimits { + param( + [string]$Body, + [int]$MaxCommentUrls, + [int]$MaxCommentMentions, + [int]$MaxCommentCharacters + ) + + $counts = Get-CommentLimitCounts -Body $Body + return $counts.urls -le $MaxCommentUrls -and + $counts.mentions -le $MaxCommentMentions -and + $counts.characters -le $MaxCommentCharacters +} + +function Merge-VisualsIntoBody { + param( + [string]$Body, + [object]$Context, + [string]$Repository, + [int]$PrNumber, + [int]$MaxCommentUrls, + [int]$MaxCommentMentions, + [int]$MaxCommentCharacters + ) + + if ($null -eq $Body) { + $Body = "" + } + + $baseBody = Remove-InlineVisualSection -Body $Body + $placeholder = Get-InlineVisualPlaceholder + if (-not $Context.visualAssets -or -not [bool]$Context.visualAssets.published) { + return $baseBody.Replace($placeholder, "") + } + + $assetCommit = [string]$Context.visualAssets.commit + if ($assetCommit -notmatch '^[0-9a-fA-F]{40}$') { + return $baseBody.Replace($placeholder, "") + } + + $comparisons = @($Context.visualAssets.comparisons | Where-Object { $null -ne $_ }) + if ($comparisons.Count -eq 0) { + return $baseBody.Replace($placeholder, "") + } + + $validPanels = New-Object System.Collections.Generic.List[string] + $invalidCount = 0 + foreach ($comparison in $comparisons) { + $actualUrl = [string]$comparison.actualUrl + if (-not (Test-VisualAssetUrl ` + -Url $actualUrl ` + -Repository $Repository ` + -PrNumber $PrNumber ` + -AssetCommit $assetCommit)) { + $invalidCount++ + continue + } + + $baselineUrl = [string]$comparison.baselineUrl + if ($baselineUrl -and -not (Test-VisualAssetUrl ` + -Url $baselineUrl ` + -Repository $Repository ` + -PrNumber $PrNumber ` + -AssetCommit $assetCommit)) { + $baselineUrl = $null + } + + $diffUrl = [string]$comparison.diffUrl + if ($diffUrl -and -not (Test-VisualAssetUrl ` + -Url $diffUrl ` + -Repository $Repository ` + -PrNumber $PrNumber ` + -AssetCommit $assetCommit)) { + $diffUrl = $null + } + + $validPanels.Add((New-InlineVisualPanel ` + -Comparison $comparison ` + -BaselineUrl $baselineUrl ` + -ActualUrl $actualUrl ` + -DiffUrl $diffUrl)) + } + + $publisherOmitted = if ($Context.visualAssets.omittedCount) { + [Math]::Max(0, [int]$Context.visualAssets.omittedCount) + } + else { + 0 + } + $selectedPanels = New-Object System.Collections.Generic.List[string] + foreach ($panel in $validPanels) { + $trialPanels = @($selectedPanels.ToArray()) + @($panel) + $trialOmitted = $publisherOmitted + $invalidCount + ($validPanels.Count - $trialPanels.Count) + $trialSection = New-InlineVisualSection -Panels $trialPanels -OmittedCount $trialOmitted + $trialBody = Insert-InlineVisualSection -Body $baseBody -Section $trialSection + if (Test-CommentWithinLimits ` + -Body $trialBody ` + -MaxCommentUrls $MaxCommentUrls ` + -MaxCommentMentions $MaxCommentMentions ` + -MaxCommentCharacters $MaxCommentCharacters) { + $selectedPanels.Add($panel) + } + } + + $omittedCount = $publisherOmitted + $invalidCount + ($validPanels.Count - $selectedPanels.Count) + $section = New-InlineVisualSection -Panels $selectedPanels.ToArray() -OmittedCount $omittedCount + $mergedBody = Insert-InlineVisualSection -Body $baseBody -Section $section + if (-not (Test-CommentWithinLimits ` + -Body $mergedBody ` + -MaxCommentUrls $MaxCommentUrls ` + -MaxCommentMentions $MaxCommentMentions ` + -MaxCommentCharacters $MaxCommentCharacters)) { + return $baseBody.Replace($placeholder, "") + } + + return $mergedBody +} + +function Write-AtomicUtf8Text { + param( + [string]$Path, + [string]$Content + ) + + $directory = Split-Path -Parent $Path + if (-not $directory) { + $directory = (Get-Location).Path + } + $temporaryPath = Join-Path $directory ".$([System.IO.Path]::GetFileName($Path)).$([Guid]::NewGuid().ToString('N')).tmp" + try { + [System.IO.File]::WriteAllText( + $temporaryPath, + $Content, + [System.Text.UTF8Encoding]::new($false)) + [System.IO.File]::Move($temporaryPath, $Path, $true) + } + finally { + Remove-Item -LiteralPath $temporaryPath -Force -ErrorAction SilentlyContinue + } +} + +function Update-AgentOutputFile { + param( + [string]$Path, + [object]$Context, + [string]$Repository, + [int]$PrNumber, + [int]$MaxCommentUrls, + [int]$MaxCommentMentions, + [int]$MaxCommentCharacters + ) + + $originalJson = Get-Content -LiteralPath $Path -Raw -Encoding UTF8 + $agentOutput = $originalJson | ConvertFrom-Json + if ($null -eq $agentOutput -or $null -eq $agentOutput.items) { + return [pscustomobject]@{ changed = $false; mergedComments = 0 } + } + + $mergedComments = 0 + foreach ($item in @($agentOutput.items)) { + if ([string]$item.type -ne "add_comment" -or + [int]$item.item_number -ne $PrNumber -or + $null -eq $item.body) { + continue + } + + $originalBody = [string]$item.body + $mergedBody = Merge-VisualsIntoBody ` + -Body $originalBody ` + -Context $Context ` + -Repository $Repository ` + -PrNumber $PrNumber ` + -MaxCommentUrls $MaxCommentUrls ` + -MaxCommentMentions $MaxCommentMentions ` + -MaxCommentCharacters $MaxCommentCharacters + if ($mergedBody -ne $originalBody) { + $item.body = $mergedBody + $mergedComments++ + } + } + + if ($mergedComments -eq 0) { + return [pscustomobject]@{ changed = $false; mergedComments = 0 } + } + + $updatedJson = $agentOutput | ConvertTo-Json -Depth 100 -Compress + $roundTripped = $updatedJson | ConvertFrom-Json + if ($null -eq $roundTripped -or + @($roundTripped.items).Count -ne @($agentOutput.items).Count) { + throw "Updated agent output did not preserve the item schema." + } + foreach ($item in @($roundTripped.items)) { + if ([string]$item.type -eq "add_comment" -and [int]$item.item_number -eq $PrNumber) { + if (-not (Test-CommentWithinLimits ` + -Body ([string]$item.body) ` + -MaxCommentUrls $MaxCommentUrls ` + -MaxCommentMentions $MaxCommentMentions ` + -MaxCommentCharacters $MaxCommentCharacters)) { + throw "Merged add_comment body exceeded the configured safety limits." + } + } + } + + Write-AtomicUtf8Text -Path $Path -Content $updatedJson + return [pscustomobject]@{ changed = $true; mergedComments = $mergedComments } +} + +function Update-CommentBodyFile { + param( + [string]$Path, + [object]$Context, + [string]$Repository, + [int]$PrNumber, + [int]$MaxCommentUrls, + [int]$MaxCommentMentions, + [int]$MaxCommentCharacters + ) + + $originalBody = Get-Content -LiteralPath $Path -Raw -Encoding UTF8 + $mergedBody = Merge-VisualsIntoBody ` + -Body $originalBody ` + -Context $Context ` + -Repository $Repository ` + -PrNumber $PrNumber ` + -MaxCommentUrls $MaxCommentUrls ` + -MaxCommentMentions $MaxCommentMentions ` + -MaxCommentCharacters $MaxCommentCharacters + if ($mergedBody -eq $originalBody) { + return [pscustomobject]@{ changed = $false; mergedComments = 0 } + } + + Write-AtomicUtf8Text -Path $Path -Content $mergedBody + return [pscustomobject]@{ changed = $true; mergedComments = 1 } +} + +if ([string]::IsNullOrWhiteSpace($Repository)) { + $Repository = "dotnet/maui" +} +if ($Repository -notmatch '^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$') { + throw "Repository must be an owner/name pair." +} +if (-not (Test-Path -LiteralPath $ContextJsonPath)) { + throw "Context JSON was not found: $ContextJsonPath" +} + +$context = Get-Content -LiteralPath $ContextJsonPath -Raw -Encoding UTF8 | ConvertFrom-Json +if ($null -eq $context) { + throw "Context JSON was empty." +} +if ([string]$context.repository -ne $Repository) { + throw "Context repository '$($context.repository)' did not match trusted repository '$Repository'." +} +if ([int]$context.pr.number -ne $PrNumber) { + throw "Context PR '$($context.pr.number)' did not match trusted PR '$PrNumber'." +} + +if ($PSCmdlet.ParameterSetName -eq "AgentOutput") { + if (-not (Test-Path -LiteralPath $AgentOutputPath)) { + Write-Host "Agent output was not found; leaving the ordinary analysis unchanged." + exit 0 + } + $result = Update-AgentOutputFile ` + -Path $AgentOutputPath ` + -Context $context ` + -Repository $Repository ` + -PrNumber $PrNumber ` + -MaxCommentUrls $MaxCommentUrls ` + -MaxCommentMentions $MaxCommentMentions ` + -MaxCommentCharacters $MaxCommentCharacters +} +else { + if (-not (Test-Path -LiteralPath $CommentBodyPath)) { + throw "Comment body was not found: $CommentBodyPath" + } + $result = Update-CommentBodyFile ` + -Path $CommentBodyPath ` + -Context $context ` + -Repository $Repository ` + -PrNumber $PrNumber ` + -MaxCommentUrls $MaxCommentUrls ` + -MaxCommentMentions $MaxCommentMentions ` + -MaxCommentCharacters $MaxCommentCharacters +} + +if ($result.changed) { + Write-Host "Merged trusted visual comparisons into $($result.mergedComments) analysis comment payload(s)." +} +else { + Write-Host "No visual comparison merge was needed; the ordinary analysis remains unchanged." +} diff --git a/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 b/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 index a186a4c1b758..c05605ff249c 100644 --- a/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 +++ b/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 @@ -18,10 +18,7 @@ BeforeAll { 'Test-AzDoAttachmentUrl', 'Get-SnapshotRoot', 'Get-SnapshotCandidatePaths', - 'New-VisualComparisonsMarkdown', - 'Get-ExistingVisualComments', - 'New-VisualCommentBody', - 'Publish-VisualComment' + 'New-VisualComparisonsMarkdown' )) { $function = $ast.Find({ $args[0] -is [System.Management.Automation.Language.FunctionDefinitionAst] -and @@ -154,98 +151,3 @@ Describe 'Visual comparison markdown' { ([regex]::Matches($markdown, '
').Count) | Should -Be ([regex]::Matches($markdown, '
').Count) } } - -Describe 'Visual companion comment' { - It 'wraps trusted comparison panels in the stable marker and commit header' { - $context = [pscustomobject]@{ - repository = 'dotnet/maui' - pr = [pscustomobject]@{ - author = 'octocat' - headRefOid = '0123456789abcdef0123456789abcdef01234567' - } - } - $panels = '
Comparison
' - - $body = New-VisualCommentBody -Context $context -Markdown $panels - - $body | Should -Match '' - $body | Should -Match '@octocat' - $body | Should -Match '\[`0123456`\]\(https://github\.com/dotnet/maui/commit/0123456789abcdef0123456789abcdef01234567\)' - $body | Should -Match ([regex]::Escape($panels)) - $body | Should -Match 'AI-generated visual evidence by GitHub Copilot\.' - } - - It 'creates the companion comment when no marker comment exists' { - Mock Get-ExistingVisualComments { @() } - Mock Invoke-GhApiJson { - [pscustomobject]@{ html_url = 'https://github.com/dotnet/maui/pull/123#issuecomment-1' } - } - - $url = Publish-VisualComment -Repository 'dotnet/maui' -PrNumber 123 -Body 'body' - - $url | Should -Be 'https://github.com/dotnet/maui/pull/123#issuecomment-1' - Should -Invoke Invoke-GhApiJson -Times 1 -Exactly -ParameterFilter { - $Method -eq 'POST' -and - $Endpoint -eq 'repos/dotnet/maui/issues/123/comments' -and - $Body.body -eq 'body' - } - } - - It 'updates the newest marker comment instead of creating a duplicate' { - Mock Get-ExistingVisualComments { - @( - [pscustomobject]@{ id = 10 }, - [pscustomobject]@{ id = 20 } - ) - } - Mock Invoke-GhApiJson { - [pscustomobject]@{ html_url = 'https://github.com/dotnet/maui/pull/123#issuecomment-20' } - } - - $url = Publish-VisualComment -Repository 'dotnet/maui' -PrNumber 123 -Body 'replacement' - - $url | Should -Be 'https://github.com/dotnet/maui/pull/123#issuecomment-20' - Should -Invoke Invoke-GhApiJson -Times 1 -Exactly -ParameterFilter { - $Method -eq 'PATCH' -and - $Endpoint -eq 'repos/dotnet/maui/issues/comments/20' -and - $Body.body -eq 'replacement' - } - } - - It 'falls back to an older marker when the newest matching comment is not editable' { - Mock Get-ExistingVisualComments { - @( - [pscustomobject]@{ id = 10 }, - [pscustomobject]@{ id = 20 } - ) - } - Mock Invoke-GhApiJson { - if ($Endpoint -eq 'repos/dotnet/maui/issues/comments/20') { - throw 'forbidden' - } - [pscustomobject]@{ html_url = 'https://github.com/dotnet/maui/pull/123#issuecomment-10' } - } - - $url = Publish-VisualComment -Repository 'dotnet/maui' -PrNumber 123 -Body 'replacement' - - $url | Should -Be 'https://github.com/dotnet/maui/pull/123#issuecomment-10' - Should -Invoke Invoke-GhApiJson -Times 2 -Exactly -ParameterFilter { $Method -eq 'PATCH' } - Should -Invoke Invoke-GhApiJson -Times 0 -Exactly -ParameterFilter { $Method -eq 'POST' } - } - - It 'does not create an empty-state comment when update-only finds no marker' { - Mock Get-ExistingVisualComments { @() } - Mock Invoke-GhApiJson { - throw 'unexpected API call' - } - - $url = Publish-VisualComment ` - -Repository 'dotnet/maui' ` - -PrNumber 123 ` - -Body 'no visual failures' ` - -UpdateOnly - - $url | Should -BeNullOrEmpty - Should -Invoke Invoke-GhApiJson -Times 0 -Exactly - } -} diff --git a/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 b/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 index e267abe8977b..ed8e910d3cd9 100644 --- a/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 +++ b/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 @@ -6,7 +6,8 @@ .DESCRIPTION Reads visual evidence gathered from public AzDO APIs, downloads only validated PNG attachments and the exact snapshot baseline from the tested merge commit, uploads - them to a dedicated GitHub branch, and writes deterministic collapsed comparison panels. + them to a dedicated GitHub branch, and records immutable URLs for deterministic + insertion into the single /review tests analysis comment. Visual publishing is supplementary evidence. Failures are recorded in context.json and never change the deterministic merge-readiness gate. @@ -29,9 +30,6 @@ param( [Parameter(Mandatory = $false)] [string]$AssetBranch = "review-tests-assets", - [Parameter(Mandatory = $false)] - [switch]$PostComment, - [Parameter(Mandatory = $false)] [ValidateRange(1, 50)] [int]$MaxComparisons = 24, @@ -569,102 +567,6 @@ $descriptionLine return $builder.ToString() } -function Get-ExistingVisualComments { - param( - [string]$Repository, - [int]$PrNumber - ) - - $comments = New-Object System.Collections.Generic.List[object] - for ($page = 1; $page -le 20; $page++) { - $pageComments = @(Invoke-GhApiJson ` - -Method "GET" ` - -Endpoint "repos/$Repository/issues/$PrNumber/comments?per_page=100&page=$page") - foreach ($comment in $pageComments) { - if ([string]$comment.body -and - [string]$comment.body -like '**') { - $comments.Add($comment) - } - } - if ($pageComments.Count -lt 100) { - break - } - } - return $comments.ToArray() -} - -function Publish-VisualComment { - param( - [string]$Repository, - [int]$PrNumber, - [string]$Body, - [switch]$UpdateOnly - ) - - $existing = @(Get-ExistingVisualComments -Repository $Repository -PrNumber $PrNumber | Sort-Object id -Descending) - foreach ($comment in $existing) { - try { - $result = Invoke-GhApiJson ` - -Method "PATCH" ` - -Endpoint "repos/$Repository/issues/comments/$($comment.id)" ` - -Body @{ body = $Body } - return [string]$result.html_url - } - catch { - Write-Warning "Could not update visual comment $($comment.id); trying an older marker comment." - } - } - - if ($UpdateOnly) { - if ($existing.Count -gt 0) { - throw "No existing visual marker comment was editable." - } - return $null - } - - $result = Invoke-GhApiJson ` - -Method "POST" ` - -Endpoint "repos/$Repository/issues/$PrNumber/comments" ` - -Body @{ body = $Body } - return [string]$result.html_url -} - -function New-VisualCommentBody { - param( - [object]$Context, - [string]$Markdown - ) - - $author = [string]$Context.pr.author - if ($author -notmatch '^[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?$') { - $author = $null - } - $headSha = [string]$Context.pr.headRefOid - if ($headSha -notmatch '^[0-9a-fA-F]{40}$') { - $headSha = $null - } - $sha7 = if ($headSha.Length -ge 7) { $headSha.Substring(0, 7) } else { "unknown" } - $commitUrl = if ($headSha) { "https://github.com/$($Context.repository)/commit/$headSha" } else { "#" } - $attribution = if ($author) { - "> @$author - visual test-failure comparisons are available based on commit [``$sha7``]($commitUrl)." - } - else { - "> Visual test-failure comparisons are available based on commit [``$sha7``]($commitUrl)." - } - - return @" - - -## Visual Failure Comparisons - -$attribution - -$Markdown - -AI-generated visual evidence by GitHub Copilot. -"@ -} - function Save-Context { param( [object]$Context, @@ -686,30 +588,10 @@ $visualEvidence = @($context.visualEvidence.comparisons) if ($visualEvidence.Count -eq 0) { Write-Host "No visual snapshot failures were detected." Remove-Item -LiteralPath $OutputMarkdownPath -Force -ErrorAction SilentlyContinue - $noVisualErrors = New-Object System.Collections.Generic.List[string] - if ($PostComment) { - try { - $body = @' - - -## Visual Failure Comparisons - -No visual snapshot failures were detected by the latest `/review tests` run. -'@ - Publish-VisualComment ` - -Repository $Repository ` - -PrNumber $PrNumber ` - -Body $body ` - -UpdateOnly | Out-Null - } - catch { - $noVisualErrors.Add("Existing visual comparison comment could not be updated: $($_.Exception.Message)") - } - } $context | Add-Member -NotePropertyName visualAssets -NotePropertyValue ([ordered]@{ published = $false comparisonCount = 0 - errors = $noVisualErrors.ToArray() + errors = @() }) -Force Save-Context -Context $context -Path $ContextJsonPath exit 0 @@ -922,17 +804,6 @@ try { Set-Content -LiteralPath $OutputMarkdownPath -Value $markdown -Encoding UTF8 } - $commentUrl = $null - if ($PostComment) { - try { - $commentBody = New-VisualCommentBody -Context $context -Markdown $markdown - $commentUrl = Publish-VisualComment -Repository $Repository -PrNumber $PrNumber -Body $commentBody - } - catch { - $errors.Add("Visual comparison comment could not be posted: $($_.Exception.Message)") - } - } - $context | Add-Member -NotePropertyName visualAssets -NotePropertyValue ([ordered]@{ published = $true branch = $AssetBranch @@ -940,7 +811,6 @@ try { comparisonCount = $published.Count omittedCount = $omittedCount markdownPath = [System.IO.Path]::GetFileName($OutputMarkdownPath) - commentUrl = $commentUrl comparisons = $published.ToArray() errors = $errors.ToArray() }) -Force diff --git a/.github/workflows/copilot-review-tests.lock.yml b/.github/workflows/copilot-review-tests.lock.yml index 8efc7931ab5a..7027457a4211 100644 --- a/.github/workflows/copilot-review-tests.lock.yml +++ b/.github/workflows/copilot-review-tests.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"90b2c600ea84962711207512946a60fc430f5db4af07b9da6575e19f92ff833e","body_hash":"114b4f90e4da8d71fb167b5ff52d113e5e90c7b1aacb48afa9f4d9f650c2f330","compiler_version":"v0.80.9","strict":true,"agent_id":"copilot","agent_model":"claude-opus-4.8","engine_versions":{"copilot":"1.0.63"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"887e2a79af7693df3e5989adcb72100a817152e14255b803f94ca304a79d7f07","body_hash":"081872d8e90fea972a1b779b650b931cd5cfafe31f20964c480652c4f640600d","compiler_version":"v0.80.9","strict":true,"agent_id":"copilot","agent_model":"claude-opus-4.8","engine_versions":{"copilot":"1.0.63"}} # gh-aw-manifest: {"version":1,"secrets":["COPILOT_PAT_0","COPILOT_PAT_1","COPILOT_PAT_2","COPILOT_PAT_3","COPILOT_PAT_4","COPILOT_PAT_5","COPILOT_PAT_6","COPILOT_PAT_7","COPILOT_PAT_8","COPILOT_PAT_9","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/cache/save","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/checkout","sha":"34e114876b0b11c390a56381ad16ebd13914f8d5","version":"v4"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/checkout","sha":"de0fac2e4500dabe0009e67214ff5f5447ce83dd","version":"v6.0.2"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"8c7d04ebf1ece56cd381446125da3e0f6896294a","version":"v0.80.9"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.7","digest":"sha256:aae231e4635c8999d039c132f1602d3df850fe9b84a00aa2b5ac981179b5661c","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.7@sha256:aae231e4635c8999d039c132f1602d3df850fe9b84a00aa2b5ac981179b5661c"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.7","digest":"sha256:009caf2e3d88fa77b64e9a03a95a228fc58db0f1701c6d324b29ba5a3c7c79b6","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.7@sha256:009caf2e3d88fa77b64e9a03a95a228fc58db0f1701c6d324b29ba5a3c7c79b6"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.7","digest":"sha256:deb1d4e19de62d51cee0508057a596a19315c3423ada4d675cad136dc8037c96","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.7@sha256:deb1d4e19de62d51cee0508057a596a19315c3423ada4d675cad136dc8037c96"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.27","digest":"sha256:fe984bddde4ec05d756d9043edb0a32912e6b7b72f6a121b1082f29221421cc7","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.3.27@sha256:fe984bddde4ec05d756d9043edb0a32912e6b7b72f6a121b1082f29221421cc7"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.4.0","digest":"sha256:2afb26356481d1a350e14544a6e160f7f7ec1561a1ea309b823665abf0309036","pinned_image":"ghcr.io/github/github-mcp-server:v1.4.0@sha256:2afb26356481d1a350e14544a6e160f7f7ec1561a1ea309b823665abf0309036"}]} # This file was automatically generated by gh-aw (v0.80.9). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # @@ -152,7 +152,7 @@ on: # PR_NUMBER: ${{ github.event.issue.number || inputs.pr_number }} # if: steps.exact_command.outputs.should_run == 'true' && (github.event_name == 'workflow_dispatch' || steps.authorization.outputs.authorized == 'true') && (github.event_name != 'workflow_dispatch' || inputs.suppress_output != true) # name: Publish visual comparison assets - # run: "set -euo pipefail\ncontext=\"CustomAgentLogsTmp/TestFailureReview/${PR_NUMBER}/context.json\"\noutput=\"CustomAgentLogsTmp/TestFailureReview/${PR_NUMBER}/visual-comparisons.md\"\npwsh .github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 \\\n -PrNumber \"${PR_NUMBER}\" \\\n -ContextJsonPath \"${context}\" \\\n -OutputMarkdownPath \"${output}\" \\\n -PostComment\n" + # run: "set -euo pipefail\ncontext=\"CustomAgentLogsTmp/TestFailureReview/${PR_NUMBER}/context.json\"\noutput=\"CustomAgentLogsTmp/TestFailureReview/${PR_NUMBER}/visual-comparisons.md\"\npwsh .github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 \\\n -PrNumber \"${PR_NUMBER}\" \\\n -ContextJsonPath \"${context}\" \\\n -OutputMarkdownPath \"${output}\"\n" # - if: steps.exact_command.outputs.should_run == 'true' && (github.event_name == 'workflow_dispatch' || steps.authorization.outputs.authorized == 'true') # name: Upload test-failure context # uses: actions/upload-artifact@v7.0.1 @@ -615,6 +615,11 @@ jobs: with: name: review-tests-context-${{ github.run_id }} path: /tmp/gh-aw/agent/review-tests-context-${{ github.run_id }}/${{ github.event.issue.number || inputs.pr_number }} + - env: + CONTEXT_PATH: /tmp/gh-aw/agent/review-tests-context-${{ github.run_id }}/${{ github.event.issue.number || inputs.pr_number }}/context.json + PR_NUMBER: ${{ github.event.issue.number || inputs.pr_number }} + name: Seal trusted visual merger inputs + run: "set -euo pipefail\ntrusted=\"${RUNNER_TEMP}/review-tests-trusted-${GITHUB_RUN_ID}-${PR_NUMBER}\"\nsudo install -d -o root -g root -m 0555 \"${trusted}\"\nsudo install -o root -g root -m 0444 \"${CONTEXT_PATH}\" \"${trusted}/context.json\"\nsudo install -o root -g root -m 0555 \\\n .github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 \\\n \"${trusted}/Merge-TestVisualsIntoComment.ps1\"\n" - name: Configure Git credentials env: @@ -1144,6 +1149,13 @@ jobs: if [ ! -f /tmp/gh-aw/agent_output.json ]; then echo '{"items":[]}' > /tmp/gh-aw/agent_output.json fi + - continue-on-error: true + env: + PR_NUMBER: ${{ github.event.issue.number || inputs.pr_number }} + if: always() + name: Merge trusted visuals into the analysis comment + run: "set -euo pipefail\ntrusted=\"${RUNNER_TEMP}/review-tests-trusted-${GITHUB_RUN_ID}-${PR_NUMBER}\"\nagent_output=\"/tmp/gh-aw/agent_output.json\"\nif [ ! -f \"${agent_output}\" ] || [ ! -f \"${trusted}/context.json\" ]; then\n echo \"No agent comment payload or trusted visual context was available; leaving the ordinary analysis unchanged.\"\n exit 0\nfi\nunset COPILOT_GITHUB_TOKEN GH_TOKEN GITHUB_TOKEN\npwsh \"${trusted}/Merge-TestVisualsIntoComment.ps1\" \\\n -PrNumber \"${PR_NUMBER}\" \\\n -Repository \"${GITHUB_REPOSITORY}\" \\\n -ContextJsonPath \"${trusted}/context.json\" \\\n -AgentOutputPath \"${agent_output}\"\n" + - name: Upload agent artifacts if: always() continue-on-error: true @@ -1909,8 +1921,7 @@ jobs: pwsh .github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 \ -PrNumber "${PR_NUMBER}" \ -ContextJsonPath "${context}" \ - -OutputMarkdownPath "${output}" \ - -PostComment + -OutputMarkdownPath "${output}" env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.issue.number || inputs.pr_number }} diff --git a/.github/workflows/copilot-review-tests.md b/.github/workflows/copilot-review-tests.md index b2bf045cd93b..e8fcb059975d 100644 --- a/.github/workflows/copilot-review-tests.md +++ b/.github/workflows/copilot-review-tests.md @@ -150,8 +150,7 @@ on: pwsh .github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 \ -PrNumber "${PR_NUMBER}" \ -ContextJsonPath "${context}" \ - -OutputMarkdownPath "${output}" \ - -PostComment + -OutputMarkdownPath "${output}" - name: Upload test-failure context if: >- steps.exact_command.outputs.should_run == 'true' && @@ -282,6 +281,39 @@ steps: with: name: review-tests-context-${{ github.run_id }} path: /tmp/gh-aw/agent/review-tests-context-${{ github.run_id }}/${{ github.event.issue.number || inputs.pr_number }} + - name: Seal trusted visual merger inputs + env: + PR_NUMBER: ${{ github.event.issue.number || inputs.pr_number }} + CONTEXT_PATH: /tmp/gh-aw/agent/review-tests-context-${{ github.run_id }}/${{ github.event.issue.number || inputs.pr_number }}/context.json + run: | + set -euo pipefail + trusted="${RUNNER_TEMP}/review-tests-trusted-${GITHUB_RUN_ID}-${PR_NUMBER}" + sudo install -d -o root -g root -m 0555 "${trusted}" + sudo install -o root -g root -m 0444 "${CONTEXT_PATH}" "${trusted}/context.json" + sudo install -o root -g root -m 0555 \ + .github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 \ + "${trusted}/Merge-TestVisualsIntoComment.ps1" + +post-steps: + - name: Merge trusted visuals into the analysis comment + if: always() + continue-on-error: true + env: + PR_NUMBER: ${{ github.event.issue.number || inputs.pr_number }} + run: | + set -euo pipefail + trusted="${RUNNER_TEMP}/review-tests-trusted-${GITHUB_RUN_ID}-${PR_NUMBER}" + agent_output="/tmp/gh-aw/agent_output.json" + if [ ! -f "${agent_output}" ] || [ ! -f "${trusted}/context.json" ]; then + echo "No agent comment payload or trusted visual context was available; leaving the ordinary analysis unchanged." + exit 0 + fi + unset COPILOT_GITHUB_TOKEN GH_TOKEN GITHUB_TOKEN + pwsh "${trusted}/Merge-TestVisualsIntoComment.ps1" \ + -PrNumber "${PR_NUMBER}" \ + -Repository "${GITHUB_REPOSITORY}" \ + -ContextJsonPath "${trusted}/context.json" \ + -AgentOutputPath "${agent_output}" --- # Review PR Test Failures @@ -310,9 +342,9 @@ The deterministic gather step wrote these files: - `/tmp/gh-aw/agent/review-tests-context-${{ github.run_id }}/${{ github.event.issue.number || inputs.pr_number }}/context.json` - `/tmp/gh-aw/agent/review-tests-context-${{ github.run_id }}/${{ github.event.issue.number || inputs.pr_number }}/context.md` -Read both files before classifying failures. When `context.json` contains a non-empty -`visualAssets.commentUrl`, include one link to that trusted companion comment where -indicated below. Do not embed the individual image URLs in the main report. +Read both files before classifying failures. `visualAssets` may describe trusted, +immutable visual images, but do not reproduce its URLs or render visual panels yourself. +A deterministic post-step inserts a bounded visual section into your one comment payload. ## Pre-flight check @@ -325,7 +357,7 @@ test -f '/tmp/gh-aw/agent/review-tests-context-${{ github.run_id }}/${{ github.e test -f '/tmp/gh-aw/agent/review-tests-context-${{ github.run_id }}/${{ github.event.issue.number || inputs.pr_number }}/context.md' ``` -The visual companion comment is optional. Its absence must not block the ordinary +Visual asset publication is optional. Its absence must not block the ordinary test-failure report or change the deterministic verdict ceiling. If required files are missing, post a short failure report with `add_comment` unless dry-run mode is active. @@ -374,8 +406,7 @@ If dry-run mode is not active, call `add_comment` exactly once with `item_number **Builds (this PR):** [build definition + ID links]. **Base sampling ([base branch], [N] recent build(s) per definition — the actual `baseSampleCount`):** [recent base build ID links]. -[When `visualAssets.commentUrl` is non-empty, add exactly one line: -`**Visual comparisons:** [Open the expandable baseline / actual / diff panels]([visualAssets.commentUrl]).`] + ### Recommended action @@ -396,8 +427,10 @@ Do not use colorful emojis anywhere in the posted comment; the only status glyph Use Markdown links, not raw `` tags. gh-aw safe outputs sanitize raw anchors before posting. -Do not embed or reproduce the individual visual image URLs in this comment. The trusted -pre-activation step already posts the bounded companion comment. Visual evidence is -supplementary only and never permits a verdict above `gate.verdictCeiling`. +Do not embed, link, summarize, or reproduce individual visual images yourself. Emit the +`` placeholder exactly once inside the main collapsible. +A trusted post-step replaces it with bounded expandable panels in this same comment. +Visual evidence is supplementary only and never permits a verdict above +`gate.verdictCeiling`. Do not use `
` anywhere. Every collapsible section must be collapsed by default. From 87ce58d0527df1f301956142412ed950e2ec71a8 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Mon, 20 Jul 2026 18:07:34 +0200 Subject: [PATCH 03/31] Label visual failures by PR relationship Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a280b482-e102-4ca0-9ff9-1cfe1946e21f --- .github/docs/maui-ci-facts.md | 6 +- .github/skills/review-test-failures/SKILL.md | 8 +- .../Merge-TestVisualsIntoComment.Tests.ps1 | 93 ++++++++++++++++++- .../scripts/Merge-TestVisualsIntoComment.ps1 | 74 ++++++++++++++- 4 files changed, 174 insertions(+), 7 deletions(-) diff --git a/.github/docs/maui-ci-facts.md b/.github/docs/maui-ci-facts.md index f57ca89018cf..c8b0f0c6c2fa 100644 --- a/.github/docs/maui-ci-facts.md +++ b/.github/docs/maui-ci-facts.md @@ -276,7 +276,11 @@ complete expandable comparison panels as fit inside the single test-failure anal comment while enforcing gh-aw's URL, mention, and character limits; excess panels are reported as omitted rather than creating another comment. Visual publishing is supplementary evidence only: missing images never raise or lower the deterministic -verdict ceiling. +verdict ceiling. Each panel also shows a conservative relationship label derived only +from the exact test-and-platform `deterministicAttribution`: `regressed-vs-base` is +Likely PR-caused, `pre-existing-on-base` or `known-issue` is Likely unrelated, and +indeterminate or unmatched evidence remains Needs human investigation. Platform or area +mismatch alone never produces an unrelated label. ## Platform mismatch diff --git a/.github/skills/review-test-failures/SKILL.md b/.github/skills/review-test-failures/SKILL.md index eb2da6a4db91..08e166da0135 100644 --- a/.github/skills/review-test-failures/SKILL.md +++ b/.github/skills/review-test-failures/SKILL.md @@ -387,8 +387,12 @@ Rules: - Use Markdown links, not raw `` tags. gh-aw safe outputs sanitize raw anchors before posting. - Do not embed, link, or reproduce individual visual image URLs in the generated analysis. The trusted merger inserts complete expandable panels into the same final - comment while enforcing gh-aw's URL, mention, and character limits. Visual publishing - failures are limitations only; they do not weaken or raise the gate. + comment while enforcing gh-aw's URL, mention, and character limits. The merger labels + each panel from exact test-and-platform deterministic attribution only: PR-only + regressions are `Likely PR-caused`, exact base/known-issue matches are + `Likely unrelated`, and unmatched or mixed evidence remains + `Needs human investigation`. Visual publishing failures are limitations only; they do + not weaken or raise the gate. - Badge colors for the `Overall` (merge-readiness) badge: `1a7f37` for `Ready to merge` and `No failures found`, `d1242f` for `Not ready`, `bf8700` for `Needs human investigation`, and `6e7781` for `Insufficient data`. diff --git a/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.Tests.ps1 b/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.Tests.ps1 index 9bbdb8d026ce..18e1553cfd98 100644 --- a/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.Tests.ps1 +++ b/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.Tests.ps1 @@ -19,6 +19,7 @@ BeforeAll { 'Test-VisualAssetUrl', 'Get-CommentLimitCounts', 'Remove-InlineVisualSection', + 'Get-VisualRelationship', 'New-InlineVisualPanel', 'New-InlineVisualSection', 'Insert-InlineVisualSection', @@ -61,6 +62,7 @@ BeforeAll { function New-VisualTestContext { param( [object[]]$Comparisons = @((New-VisualTestComparison)), + [object[]]$Failures = @(), [int]$OmittedCount = 0, [bool]$Published = $true, [string]$Commit = ('a' * 40), @@ -70,6 +72,9 @@ BeforeAll { return [pscustomobject]@{ repository = 'dotnet/maui' pr = [pscustomobject]@{ number = $PrNumber } + failures = [pscustomobject]@{ + unique = $Failures + } visualAssets = [pscustomobject]@{ published = $Published commit = $Commit @@ -78,6 +83,24 @@ BeforeAll { } } } + + function New-VisualTestFailure { + param( + [string]$TestName = 'VisualTest', + [string]$Platform = 'ios', + [string]$DeterministicAttribution = 'indeterminate', + [bool]$AlsoFailsOnBaseline = $false, + [bool]$LegAlsoFailsOnBase = $false + ) + + return [pscustomobject]@{ + testName = $TestName + platform = $Platform + deterministicAttribution = $DeterministicAttribution + alsoFailsOnBaseline = $AlsoFailsOnBaseline + legAlsoFailsOnBase = $LegAlsoFailsOnBase + } + } } Describe 'Inline visual input validation' { @@ -131,7 +154,10 @@ Describe 'Inline visual input validation' { Describe 'Inline visual body merge' { It 'replaces the trusted placeholder with escaped expandable panels inside one comment' { $comparison = New-VisualTestComparison -TestName '@danger' - $context = New-VisualTestContext -Comparisons @($comparison) + $failure = New-VisualTestFailure ` + -TestName '@danger' ` + -DeterministicAttribution 'regressed-vs-base' + $context = New-VisualTestContext -Comparisons @($comparison) -Failures @($failure) $body = @' ## Tests Failure Analysis @@ -162,6 +188,8 @@ Investigate. $merged | Should -Match ([regex]::Escape((Get-InlineVisualStartMarker))) $merged | Should -Not -Match ([regex]::Escape((Get-InlineVisualPlaceholder))) $merged | Should -Match '<b>@danger</b>' + $merged | Should -Match 'Likely PR-caused - visual comparison' + $merged | Should -Match 'The same leg was green on the sampled base build and red on this PR' $merged | Should -Match 'CI baselineFresh PR actualCI diff' $merged.IndexOf('### Visual failure comparisons') | Should -BeLessThan $merged.IndexOf('### Recommended action') @@ -227,9 +255,9 @@ Investigate. -PrNumber 123 ` -MaxCommentUrls 45 ` -MaxCommentMentions 10 ` - -MaxCommentCharacters 1700 + -MaxCommentCharacters 2000 - $merged.Length | Should -BeLessOrEqual 1700 + $merged.Length | Should -BeLessOrEqual 2000 ([regex]::Matches($merged, '
').Count) | Should -Be ([regex]::Matches($merged, '
').Count) $merged | Should -Match 'additional comparison\(s\) were omitted' @@ -311,6 +339,65 @@ Investigate. } } +Describe 'Inline visual relationship classification' { + It 'maps exact deterministic attribution to the review taxonomy' { + $context = New-VisualTestContext -Failures @( + (New-VisualTestFailure -TestName 'Regression' -DeterministicAttribution 'regressed-vs-base'), + (New-VisualTestFailure -TestName 'Existing' -DeterministicAttribution 'pre-existing-on-base'), + (New-VisualTestFailure -TestName 'Known' -DeterministicAttribution 'known-issue'), + (New-VisualTestFailure -TestName 'Mixed' -AlsoFailsOnBaseline $true) + ) + + (Get-VisualRelationship ` + -Comparison (New-VisualTestComparison -TestName 'Regression') ` + -Context $context).label | + Should -Be 'Likely PR-caused' + (Get-VisualRelationship ` + -Comparison (New-VisualTestComparison -TestName 'Existing') ` + -Context $context).label | + Should -Be 'Likely unrelated' + (Get-VisualRelationship ` + -Comparison (New-VisualTestComparison -TestName 'Known') ` + -Context $context).label | + Should -Be 'Likely unrelated' + + $mixed = Get-VisualRelationship ` + -Comparison (New-VisualTestComparison -TestName 'Mixed') ` + -Context $context + $mixed.label | Should -Be 'Needs human investigation' + $mixed.detail | Should -Match 'not strong enough to dismiss' + } + + It 'requires an exact platform match before calling a visual failure unrelated' { + $context = New-VisualTestContext -Failures @( + (New-VisualTestFailure ` + -TestName 'VisualTest' ` + -Platform 'unknown' ` + -DeterministicAttribution 'pre-existing-on-base') + ) + + $relationship = Get-VisualRelationship ` + -Comparison (New-VisualTestComparison -Platform 'ios') ` + -Context $context + + $relationship.label | Should -Be 'Needs human investigation' + $relationship.detail | Should -Match 'No decisive exact test-and-platform' + } + + It 'does not surface an unrecognized attribution value' { + $context = New-VisualTestContext -Failures @( + (New-VisualTestFailure -DeterministicAttribution '') + ) + + $relationship = Get-VisualRelationship ` + -Comparison (New-VisualTestComparison) ` + -Context $context + + $relationship.label | Should -Be 'Needs human investigation' + $relationship.detail | Should -Not -Match 'script|@all' + } +} + Describe 'Agent output mutation' { It 'atomically updates only the matching add_comment item and preserves its schema' { $context = New-VisualTestContext diff --git a/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 b/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 index 00ecd307ae65..76cfe798a7da 100644 --- a/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 +++ b/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 @@ -172,9 +172,74 @@ function Remove-InlineVisualSection { [TimeSpan]::FromSeconds(1)) } +function Get-VisualRelationship { + param( + [object]$Comparison, + [object]$Context + ) + + $testName = [string]$Comparison.testName + $platform = [string]$Comparison.platform + $failure = $null + foreach ($candidate in @($Context.failures.unique | Where-Object { $null -ne $_ })) { + if ([string]::Equals( + [string]$candidate.testName, + $testName, + [StringComparison]::OrdinalIgnoreCase) -and + [string]::Equals( + [string]$candidate.platform, + $platform, + [StringComparison]::OrdinalIgnoreCase)) { + $failure = $candidate + break + } + } + + if ($null -eq $failure) { + return [pscustomobject]@{ + label = "Needs human investigation" + detail = "No decisive exact test-and-platform baseline attribution was available." + } + } + + switch ([string]$failure.deterministicAttribution) { + "regressed-vs-base" { + return [pscustomobject]@{ + label = "Likely PR-caused" + detail = "The same leg was green on the sampled base build and red on this PR." + } + } + "pre-existing-on-base" { + return [pscustomobject]@{ + label = "Likely unrelated" + detail = "The exact test and platform also failed on the base branch." + } + } + "known-issue" { + return [pscustomobject]@{ + label = "Likely unrelated" + detail = "The exact test and platform also failed on base and matched a known issue." + } + } + default { + $detail = if ([bool]$failure.alsoFailsOnBaseline -or [bool]$failure.legAlsoFailsOnBase) { + "Base-branch evidence exists, but it was not strong enough to dismiss this exact failure." + } + else { + "No decisive exact test-and-platform baseline attribution was available." + } + return [pscustomobject]@{ + label = "Needs human investigation" + detail = $detail + } + } + } +} + function New-InlineVisualPanel { param( [object]$Comparison, + [object]$Relationship, [string]$BaselineUrl, [string]$ActualUrl, [string]$DiffUrl @@ -188,6 +253,8 @@ function New-InlineVisualPanel { $actualAlt = Escape-VisualText -Value "$([string]$Comparison.testName) actual" -MaximumLength 220 $diffAlt = Escape-VisualText -Value "$([string]$Comparison.testName) diff" -MaximumLength 220 $safeActualUrl = Escape-VisualText -Value $ActualUrl -MaximumLength 2048 + $relationshipLabel = Escape-VisualText -Value ([string]$Relationship.label) -MaximumLength 80 + $relationshipDetail = Escape-VisualText -Value ([string]$Relationship.detail) -MaximumLength 240 $buildId = [int]$Comparison.buildId $baselineCell = if ($BaselineUrl) { @@ -213,10 +280,12 @@ function New-InlineVisualPanel { return @"
-$testName - $platform - visual comparison +$testName - $platform - $relationshipLabel - visual comparison $descriptionLine +**Relationship to PR:** **$relationshipLabel** - $relationshipDetail + @@ -238,6 +307,7 @@ function New-InlineVisualSection { [void]$builder.AppendLine("### Visual failure comparisons") [void]$builder.AppendLine() [void]$builder.AppendLine("Full-resolution CI baseline, actual, and diff images are embedded below. They supplement the failure classification and do not change the deterministic verdict ceiling.") + [void]$builder.AppendLine("Relationship labels use deterministic exact test-and-platform baseline evidence; missing or mixed evidence remains **Needs human investigation**.") [void]$builder.AppendLine() foreach ($panel in @($Panels)) { @@ -355,8 +425,10 @@ function Merge-VisualsIntoBody { $diffUrl = $null } + $relationship = Get-VisualRelationship -Comparison $comparison -Context $Context $validPanels.Add((New-InlineVisualPanel ` -Comparison $comparison ` + -Relationship $relationship ` -BaselineUrl $baselineUrl ` -ActualUrl $actualUrl ` -DiffUrl $diffUrl)) From 4ccbab1db21e605dd0ea3e0917369b52bdef6a19 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Mon, 20 Jul 2026 18:31:03 +0200 Subject: [PATCH 04/31] Use changed visual scope in relation labels Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a280b482-e102-4ca0-9ff9-1cfe1946e21f --- .github/docs/maui-ci-facts.md | 11 ++- .github/skills/review-test-failures/SKILL.md | 11 ++- .../Merge-TestVisualsIntoComment.Tests.ps1 | 72 ++++++++++++++ .../scripts/Merge-TestVisualsIntoComment.ps1 | 93 +++++++++++++++++-- .../scripts/Publish-TestVisualAssets.ps1 | 4 + 5 files changed, 173 insertions(+), 18 deletions(-) diff --git a/.github/docs/maui-ci-facts.md b/.github/docs/maui-ci-facts.md index c8b0f0c6c2fa..d637136a19cb 100644 --- a/.github/docs/maui-ci-facts.md +++ b/.github/docs/maui-ci-facts.md @@ -276,11 +276,12 @@ complete expandable comparison panels as fit inside the single test-failure anal comment while enforcing gh-aw's URL, mention, and character limits; excess panels are reported as omitted rather than creating another comment. Visual publishing is supplementary evidence only: missing images never raise or lower the deterministic -verdict ceiling. Each panel also shows a conservative relationship label derived only -from the exact test-and-platform `deterministicAttribution`: `regressed-vs-base` is -Likely PR-caused, `pre-existing-on-base` or `known-issue` is Likely unrelated, and -indeterminate or unmatched evidence remains Needs human investigation. Platform or area -mismatch alone never produces an unrelated label. +verdict ceiling. Each panel also shows a conservative relationship label derived from +the exact test-and-platform `deterministicAttribution` plus exact changed snapshot/test +scope: `regressed-vs-base` or directly changed visual coverage is Likely PR-caused, +`pre-existing-on-base` or `known-issue` is Likely unrelated, and indeterminate or +unmatched evidence remains Needs human investigation. A same-named snapshot on another +platform and platform or area mismatch alone never change the label. ## Platform mismatch diff --git a/.github/skills/review-test-failures/SKILL.md b/.github/skills/review-test-failures/SKILL.md index 08e166da0135..eb4565ad24a7 100644 --- a/.github/skills/review-test-failures/SKILL.md +++ b/.github/skills/review-test-failures/SKILL.md @@ -388,11 +388,12 @@ Rules: - Do not embed, link, or reproduce individual visual image URLs in the generated analysis. The trusted merger inserts complete expandable panels into the same final comment while enforcing gh-aw's URL, mention, and character limits. The merger labels - each panel from exact test-and-platform deterministic attribution only: PR-only - regressions are `Likely PR-caused`, exact base/known-issue matches are - `Likely unrelated`, and unmatched or mixed evidence remains - `Needs human investigation`. Visual publishing failures are limitations only; they do - not weaken or raise the gate. + each panel from exact test-and-platform deterministic attribution plus exact changed + snapshot/test scope: PR-only regressions and directly changed visual coverage are + `Likely PR-caused`, exact base/known-issue matches are `Likely unrelated`, and + unmatched or mixed evidence remains `Needs human investigation`. A same-named snapshot + on another platform does not count as changed scope. Visual publishing failures are + limitations only; they do not weaken or raise the gate. - Badge colors for the `Overall` (merge-readiness) badge: `1a7f37` for `Ready to merge` and `No failures found`, `d1242f` for `Not ready`, `bf8700` for `Needs human investigation`, and `6e7781` for `Insufficient data`. diff --git a/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.Tests.ps1 b/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.Tests.ps1 index 18e1553cfd98..36bd05680993 100644 --- a/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.Tests.ps1 +++ b/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.Tests.ps1 @@ -19,6 +19,8 @@ BeforeAll { 'Test-VisualAssetUrl', 'Get-CommentLimitCounts', 'Remove-InlineVisualSection', + 'Test-VisualSnapshotPathMatchesPlatform', + 'Test-VisualComparisonChanged', 'Get-VisualRelationship', 'New-InlineVisualPanel', 'New-InlineVisualSection', @@ -43,6 +45,8 @@ BeforeAll { [int]$PrNumber = 123, [string]$TestName = 'VisualTest', [string]$Platform = 'ios', + [string]$SnapshotFileName, + [string]$AutomatedTestName, [switch]$ActualOnly ) @@ -50,6 +54,8 @@ BeforeAll { return [pscustomobject]@{ testName = $TestName platform = $Platform + snapshotFileName = $(if ($SnapshotFileName) { $SnapshotFileName } else { "$TestName.png" }) + automatedTestName = $AutomatedTestName description = '1.25% difference' buildId = 456 baselineStatus = 'resolved from the tested runtime environment' @@ -63,6 +69,7 @@ BeforeAll { param( [object[]]$Comparisons = @((New-VisualTestComparison)), [object[]]$Failures = @(), + [string[]]$ChangedFiles = @(), [int]$OmittedCount = 0, [bool]$Published = $true, [string]$Commit = ('a' * 40), @@ -75,6 +82,9 @@ BeforeAll { failures = [pscustomobject]@{ unique = $Failures } + scope = [pscustomobject]@{ + changedFiles = $ChangedFiles + } visualAssets = [pscustomobject]@{ published = $Published commit = $Commit @@ -396,6 +406,68 @@ Describe 'Inline visual relationship classification' { $relationship.label | Should -Be 'Needs human investigation' $relationship.detail | Should -Not -Match 'script|@all' } + + It 'marks an exact changed platform snapshot as likely PR-caused' { + $comparison = New-VisualTestComparison ` + -TestName 'ChangedSnapshot' ` + -Platform 'windows' ` + -SnapshotFileName 'ChangedSnapshot.png' + $context = New-VisualTestContext ` + -Comparisons @($comparison) ` + -Failures @( + (New-VisualTestFailure ` + -TestName 'ChangedSnapshot' ` + -Platform 'windows' ` + -DeterministicAttribution 'pre-existing-on-base') + ) ` + -ChangedFiles @( + 'src/Controls/tests/TestCases.WinUI.Tests/snapshots/windows/ChangedSnapshot.png' + ) + + $relationship = Get-VisualRelationship -Comparison $comparison -Context $context + + $relationship.label | Should -Be 'Likely PR-caused' + $relationship.detail | Should -Match 'exact snapshot or visual test' + } + + It 'does not use a same-named snapshot changed for another platform' { + $comparison = New-VisualTestComparison ` + -TestName 'CrossPlatformSnapshot' ` + -Platform 'windows' ` + -SnapshotFileName 'CrossPlatformSnapshot.png' + $context = New-VisualTestContext ` + -Comparisons @($comparison) ` + -Failures @( + (New-VisualTestFailure ` + -TestName 'CrossPlatformSnapshot' ` + -Platform 'windows' ` + -DeterministicAttribution 'pre-existing-on-base') + ) ` + -ChangedFiles @( + 'src/Controls/tests/TestCases.iOS.Tests/snapshots/ios-26/CrossPlatformSnapshot.png' + ) + + (Get-VisualRelationship -Comparison $comparison -Context $context).label | + Should -Be 'Likely unrelated' + } + + It 'marks the exact changed visual test class as likely PR-caused' { + $comparison = New-VisualTestComparison ` + -TestName 'VerifySearch' ` + -Platform 'windows' ` + -AutomatedTestName 'Microsoft.Maui.TestCases.Tests.ShellSearchHandlerFeatureTests(Windows).VerifySearch' + $context = New-VisualTestContext ` + -Comparisons @($comparison) ` + -Failures @( + (New-VisualTestFailure -TestName 'VerifySearch' -Platform 'windows') + ) ` + -ChangedFiles @( + 'src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/ShellSearchHandlerFeatureTests.cs' + ) + + (Get-VisualRelationship -Comparison $comparison -Context $context).label | + Should -Be 'Likely PR-caused' + } } Describe 'Agent output mutation' { diff --git a/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 b/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 index 76cfe798a7da..ea1cb7ec60e9 100644 --- a/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 +++ b/.github/skills/review-test-failures/scripts/Merge-TestVisualsIntoComment.ps1 @@ -172,6 +172,70 @@ function Remove-InlineVisualSection { [TimeSpan]::FromSeconds(1)) } +function Test-VisualSnapshotPathMatchesPlatform { + param( + [string]$Path, + [string]$Platform + ) + + $normalizedPath = "/" + (($Path -replace '\\', '/').TrimStart('/')) + $normalizedPlatform = if ($null -eq $Platform) { "" } else { $Platform.ToLowerInvariant() } + switch ($normalizedPlatform) { + "android" { return $normalizedPath -match '/snapshots/android(?:-[^/]+)?/' } + "ios" { return $normalizedPath -match '/snapshots/ios(?:-[^/]+)?/' } + "macos" { return $normalizedPath -match '/snapshots/mac/' } + "maccatalyst" { return $normalizedPath -match '/snapshots/mac/' } + "windows" { return $normalizedPath -match '/snapshots/windows/' } + default { return $false } + } +} + +function Test-VisualComparisonChanged { + param( + [object]$Comparison, + [object]$Context + ) + + $changedFiles = @($Context.scope.changedFiles | Where-Object { -not [string]::IsNullOrWhiteSpace([string]$_) }) + if ($changedFiles.Count -eq 0) { + return $false + } + + $snapshotFileName = [System.IO.Path]::GetFileName([string]$Comparison.snapshotFileName) + if (-not [string]::IsNullOrWhiteSpace($snapshotFileName)) { + foreach ($changedFile in $changedFiles) { + $matchingFileName = [string]::Equals( + [System.IO.Path]::GetFileName([string]$changedFile), + $snapshotFileName, + [StringComparison]::OrdinalIgnoreCase) + $matchingPlatform = Test-VisualSnapshotPathMatchesPlatform ` + -Path ([string]$changedFile) ` + -Platform ([string]$Comparison.platform) + if ($matchingFileName -and $matchingPlatform) { + return $true + } + } + } + + $automatedTestName = [string]$Comparison.automatedTestName + $classMatch = [regex]::Match( + $automatedTestName, + '\.(?[A-Za-z_][A-Za-z0-9_]*)(?:\([^)]*\))?\.[^.]+$') + if ($classMatch.Success) { + $testFileName = $classMatch.Groups['class'].Value + ".cs" + foreach ($changedFile in $changedFiles) { + if ([string]::Equals( + [System.IO.Path]::GetFileName([string]$changedFile), + $testFileName, + [StringComparison]::OrdinalIgnoreCase)) { + return $true + } + } + } + + return $false +} + function Get-VisualRelationship { param( [object]$Comparison, @@ -195,6 +259,25 @@ function Get-VisualRelationship { } } + $attribution = if ($null -ne $failure) { + [string]$failure.deterministicAttribution + } + else { + "" + } + if ($attribution -eq "regressed-vs-base") { + return [pscustomobject]@{ + label = "Likely PR-caused" + detail = "The same leg was green on the sampled base build and red on this PR." + } + } + $comparisonChanged = Test-VisualComparisonChanged -Comparison $Comparison -Context $Context + if ($comparisonChanged) { + return [pscustomobject]@{ + label = "Likely PR-caused" + detail = "This PR changes the exact snapshot or visual test." + } + } if ($null -eq $failure) { return [pscustomobject]@{ label = "Needs human investigation" @@ -202,13 +285,7 @@ function Get-VisualRelationship { } } - switch ([string]$failure.deterministicAttribution) { - "regressed-vs-base" { - return [pscustomobject]@{ - label = "Likely PR-caused" - detail = "The same leg was green on the sampled base build and red on this PR." - } - } + switch ($attribution) { "pre-existing-on-base" { return [pscustomobject]@{ label = "Likely unrelated" @@ -307,7 +384,7 @@ function New-InlineVisualSection { [void]$builder.AppendLine("### Visual failure comparisons") [void]$builder.AppendLine() [void]$builder.AppendLine("Full-resolution CI baseline, actual, and diff images are embedded below. They supplement the failure classification and do not change the deterministic verdict ceiling.") - [void]$builder.AppendLine("Relationship labels use deterministic exact test-and-platform baseline evidence; missing or mixed evidence remains **Needs human investigation**.") + [void]$builder.AppendLine("Relationship labels use deterministic exact test-and-platform baseline evidence plus exact changed snapshot/test scope; missing or mixed evidence remains **Needs human investigation**.") [void]$builder.AppendLine() foreach ($panel in @($Panels)) { diff --git a/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 b/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 index ed8e910d3cd9..533030b158e5 100644 --- a/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 +++ b/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.ps1 @@ -740,7 +740,9 @@ foreach ($evidence in $selectedEvidence) { $prepared.Add([ordered]@{ testName = $(if ($evidence.testName) { [string]$evidence.testName } else { [System.IO.Path]::GetFileNameWithoutExtension($snapshotFileName) }) + automatedTestName = [string]$evidence.automatedTestName platform = [string]$evidence.platform + snapshotFileName = $snapshotFileName description = $(if ($evidence.description) { [string]$evidence.description } else { [string]$evidence.kind }) buildId = $buildId buildUrl = [string]$evidence.buildUrl @@ -784,7 +786,9 @@ try { $rawPrefix = "https://raw.githubusercontent.com/$Repository/$assetCommit/" $published.Add([ordered]@{ testName = $comparison.testName + automatedTestName = $comparison.automatedTestName platform = $comparison.platform + snapshotFileName = $comparison.snapshotFileName description = $comparison.description buildId = $comparison.buildId buildUrl = $comparison.buildUrl From 8e606dac53af491ed48868e61acb428d13bd8b2f Mon Sep 17 00:00:00 2001 From: kubaflo Date: Tue, 21 Jul 2026 18:33:50 +0200 Subject: [PATCH 05/31] Fix report-fence parsing + visual dedup key; align review-tests auth gating Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d --- .github/scripts/Review-Tests.Tests.ps1 | 31 +++++ .github/scripts/Review-Tests.ps1 | 11 +- .../Publish-TestVisualAssets.Tests.ps1 | 66 +++------ .../scripts/Publish-TestVisualAssets.ps1 | 127 +++--------------- .../workflows/copilot-review-tests.lock.yml | 29 ++-- .github/workflows/copilot-review-tests.md | 21 ++- 6 files changed, 104 insertions(+), 181 deletions(-) diff --git a/.github/scripts/Review-Tests.Tests.ps1 b/.github/scripts/Review-Tests.Tests.ps1 index 6479ecd912d4..a105e9ba278f 100644 --- a/.github/scripts/Review-Tests.Tests.ps1 +++ b/.github/scripts/Review-Tests.Tests.ps1 @@ -122,6 +122,37 @@ Trailing assistant prose. Should -Be ([regex]::Matches($report, '').Count) } + It 'ignores stray inline backticks before an unfenced report with evidence fences' { + $content = @' +The assistant mentions an inline marker ``` before the report. + + + +## Tests Failure Analysis + +
+Review + +```text +error: sample +``` + +**Overall verdict:** Not ready + +
+ +Trailing assistant prose. +'@ + + $report = Get-EmbeddedTestFailureReport -Content $content + + $report | Should -Match '^' + $report | Should -Match '```text' + $report | Should -Match 'error: sample' + $report | Should -Match '\*\*Overall verdict:\*\* Not ready' + $report | Should -Not -Match 'Trailing assistant prose' + } + It 'reuses a complete report instead of wrapping a second title and badge section' { $content = @' Generated report: diff --git a/.github/scripts/Review-Tests.ps1 b/.github/scripts/Review-Tests.ps1 index d913e2ae0549..53fa996234f4 100644 --- a/.github/scripts/Review-Tests.ps1 +++ b/.github/scripts/Review-Tests.ps1 @@ -97,7 +97,6 @@ New-Item -ItemType Directory -Force -Path $RunDirectory | Out-Null $ContextJsonPath = Join-Path $RunDirectory "context.json" $ContextMarkdownPath = Join-Path $RunDirectory "context.md" -$VisualComparisonsPath = Join-Path $RunDirectory "visual-comparisons.md" $PromptPath = Join-Path $RunDirectory "prompt.md" $ReportPath = Join-Path $RunDirectory "report.md" $CommentPath = Join-Path $RunDirectory "comment.md" @@ -164,10 +163,11 @@ function Get-EmbeddedTestFailureReport { $prefix = $Content.Substring(0, $startIndex) $report = $Content.Substring($startIndex) - $insideCodeFence = ([regex]::Matches($prefix, '```').Count % 2) -eq 1 + $insideCodeFence = ([regex]::Matches($prefix, '(?m)^[ \t]*```').Count % 2) -eq 1 if ($insideCodeFence) { - $closingFence = $report.LastIndexOf('```', [StringComparison]::Ordinal) - if ($closingFence -ge 0) { + $closingFenceMatches = @([regex]::Matches($report, '(?m)^[ \t]*```[^`]*[ \t]*$')) + if ($closingFenceMatches.Count -gt 0) { + $closingFence = $closingFenceMatches[$closingFenceMatches.Count - 1].Index $report = $report.Substring(0, $closingFence) } } @@ -456,8 +456,7 @@ if ($PostComment -and -not $DryRun) { & pwsh $publisherScript ` -PrNumber $PRNumber ` -Repository $Repository ` - -ContextJsonPath $ContextJsonPath ` - -OutputMarkdownPath $VisualComparisonsPath + -ContextJsonPath $ContextJsonPath if ($LASTEXITCODE -ne 0) { Write-Warning "Visual comparison publishing failed; continuing with the ordinary test-failure report." } diff --git a/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 b/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 index c05605ff249c..61ce3b06edba 100644 --- a/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 +++ b/.github/skills/review-test-failures/scripts/Publish-TestVisualAssets.Tests.ps1 @@ -11,14 +11,13 @@ BeforeAll { } foreach ($functionName in @( - 'Escape-Html', 'Get-SafeAssetSlug', 'Invoke-GhApiJson', 'Test-PngFile', 'Test-AzDoAttachmentUrl', 'Get-SnapshotRoot', 'Get-SnapshotCandidatePaths', - 'New-VisualComparisonsMarkdown' + 'Get-VisualEvidenceDedupKey' )) { $function = $ast.Find({ $args[0] -is [System.Management.Automation.Language.FunctionDefinitionAst] -and @@ -107,47 +106,26 @@ Describe 'Snapshot baseline candidates' { } } -Describe 'Visual comparison markdown' { - It 'renders escaped, collapsed three-column comparison panels' { - $markdown = New-VisualComparisonsMarkdown -Comparisons @( - [pscustomobject]@{ - testName = '' - platform = 'iOS' - description = '2.08% difference' - buildId = 123 - buildUrl = 'https://dev.azure.com/example' - baselineUrl = 'https://raw.githubusercontent.com/org/repo/sha/base.png' - baselineStatus = 'resolved' - actualUrl = 'https://raw.githubusercontent.com/org/repo/sha/actual.png' - diffUrl = 'https://raw.githubusercontent.com/org/repo/sha/diff.png' - } - ) -OmittedCount 0 -MaximumCharacters 5000 - - $markdown | Should -Match '
' - $markdown | Should -Not -Match '
CI baseline
' - $markdown | Should -Match '<script>alert\(1\)</script>' - $markdown | Should -Not -Match '
CI baselineFresh PR actualCI diff
$baselineCell $actualAltFresh PR actualCI diff