Harden maui-copilot reviewer and skill validation with GPT-only models - #38040
Harden maui-copilot reviewer and skill validation with GPT-only models#38040kubaflo wants to merge 229 commits into
Conversation
Squash-rebased onto latest main. Consolidates the improved-reviewer work for the maui-copilot AzDO PR-review pipeline (definition 27723) into a single change on top of main: - Gate/build-tasks reliability: build-tasks failures are non-blocking and produce an INCONCLUSIVE gate (not a misleading FAILED); a gate TIMEOUT is non-blocking; INCONCLUSIVE reasons are clear and actionable. - Inflight PRs are reviewed against the LATEST base (base merged into PR head) and built from the PR head, not the stale base tip. - AI Summary posting: COMMENT verdicts update one issue comment in place instead of stacking; reused summary comments are un-hidden so the fresh summary is visible. - "No UI test results" guidance is gate-aware: a FAILED (or unknown) gate points at the build/gate; an explicit PASSED/SKIPPED/INCONCLUSIVE points at infrastructure, so authors are not sent down the wrong path. - Device-test runner and detection improvements supporting the deep UI stage. Shared gh-aw review-test-failures layer (visual publishing, gather context, workflow lock files, pat_pool docs) is kept at main's newer versions; the gate-aware AI-summary refinements are preserved on top. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
Two reliability fixes for the maui-copilot PR reviewer: 1. Gate-log excerpt bloat (PR #36109 rendered a 75 KB summary). Format-GateLogExcerpt treated benign iOS/mac simulator teardown spam as "error-relevant" because every `dbug: ... process: Error Domain=...` line contains the substring ": Error ". Now the excerpt strips ANSI/VT color escapes, drops simulator runtime noise (Error Domain=, Failed to terminate process, Client not entitled, RBS*ErrorDomain, No such process found, NSUnderlyingError, runningboard, dbug/trce logging), and caps the line count. The raw-tail fallback is likewise ANSI-stripped and noise-filtered. 2. Previous AI Summary not marked outdated (PR #36676). The reuse+PATCH-in-place path relied on unminimizeComment, which MauiBot's token cannot execute (FORBIDDEN in- pipeline), so a summary that a prior sweep had collapsed could never be un-hidden. Restore the expected behavior: collapse EVERY prior AI-Summary issue comment (and stale reviews) as outdated using minimizeComment — which MauiBot CAN do — then post a brand-new comment. One visible summary above a stack of collapsed "outdated" ones. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
For a PR that adds new API AND a test referencing it in the same project, reverting the fix un-compiles the test, so the gate can never establish a "fails without the fix" baseline — it previously reported INCONCLUSIVE even when the fix built and every test passed. That's an unhelpful non-verdict for legitimate new-feature PRs. Now, when the without-fix baseline build error is in the PR's OWN detected test (compile-coupling), the fix itself compiles, and every test runs and PASSES cleanly WITH the fix (no env/build/filter error, no genuine failure), the gate reports a real ✅ PASSED — "Verified (new API / feature)" — with a note that the without-fix baseline was compile-coupled. A with-fix crash/env error (e.g. #36572's SIGABRT) still keeps it INCONCLUSIVE, so the credit only applies to a genuinely clean with-fix run. Adds Write-MarkdownReport regression tests for both the credited PASS and the uncredited INCONCLUSIVE paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
When the only with-fix env-class result is a brand-new VerifyScreenshot with no
committed baseline, the report already prints a dedicated, accurate snapshot note
("no baseline yet — inconclusive, not a fix failure; commit the baseline PNG"). It
ALSO printed the generic "Could not verify — environment/infrastructure error …
comment /review to retry" classification, which is wrong for this case (retrying
never creates the baseline) and makes an expected, non-failing INCONCLUSIVE look
like an infra failure. (PR #35491: new Shell.SetBackground API + a brand-new snapshot
test → compile-coupled baseline + no-baseline snapshot.)
Guard the generic env-error classification with $nonSnapshotEnvError so it fires only
when there is a real infra error (app crash / Appium flake / empty result), not a
pure snapshot-baseline case. Adds a regression test.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…code build error When the MSBuild build-server daemon is unavailable the build falls back to an in-process build and the in-repo Maui.InTree.targets up-to-date check misfires, reporting "required MSBuild tasks are not yet built or they are out of date" even though the Build MSBuild Tasks step succeeded. The gate previously classified this as a code BUILD ERROR (→ INCONCLUSIVE, but framed as if the PR's code failed to compile). It's a gate-agent infrastructure flake — the PR's code was never actually compiled. Detect "required MSBuild tasks are not yet built or they are out of date" / "MSBuild server unavailable" BEFORE the generic build-error branch and classify as EnvError (INCONCLUSIVE, retryable) with an honest "gate infrastructure, not a code build error" message. Adds a regression test. (build 14821162, PR #36572.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…he commenter A /review with a non-existent pipeline branch (e.g. an old/renamed branch like feature/enhanced-reviewer, now improved-reviewer) made the AzDO trigger step fail with only an opaque log error — the commenter got NO feedback on the PR and assumed the command "did nothing". (PR #34903: mattleibow's /review -b feature/enhanced-reviewer.) - Validate the --branch exists in the repo BEFORE queuing the pipeline; on a missing branch, fail fast with a specific reason. - New "Report /review trigger failure to the PR" step posts a clear WARNING comment on any trigger failure (bad branch → name the right branch + valid platforms; otherwise a transient-error retry hint), so a /review never fails silently again. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
… build error
When the gate agent's `dotnet workload restore` did not take effect, the test project
fails to build with NETSDK1147 ("the following workloads must be installed: android ...
run dotnet workload restore") for EVERY project, on BOTH the without-fix and with-fix
legs. This is a gate-agent infrastructure flake — never the PR author's code — but the
gate classified it as a code BUILD ERROR (INCONCLUSIVE framed as if the PR failed to
compile). (build 14824785, PR #36572: both legs failed with 16x NETSDK1147 each and
zero CS-errors — the android workload was simply absent.)
Detect NETSDK1147 / "the following workloads must be installed" BEFORE the generic
build-error branch and classify as EnvError (INCONCLUSIVE, retryable) with an honest
"a required .NET workload was not installed on the gate agent" message. Guarded so a
genuine CS####/MAUIX#### compile error co-occurring with NETSDK1147 still wins (real
failure never masked). Adds regression tests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…dict exists
In the deferred Stage-3 deep-results post, the PRAgent phase content (gate/content.md,
code-review/content.md, …) can be absent even though the pipeline ran and handed us a
real trusted gate verdict — the content dir wasn't carried into the Stage-3 job, or the
earlier review phase produced no files. post-ai-summary-comment.ps1 then hard-threw
("No gate or phase content found", exit 1), which FAILED the Post stage AND posted
nothing: the Task-4 fallback notice never fires because Task-4 already deferred
(aiSummaryReviewId='DEFERRED', not empty). So the PR got no summary at all. (build
14829982, PR #36657: TRX deep results present, gate verdict INCONCLUSIVE, every phase
file "not found".)
When there is no phase content but the pipeline supplied a real trusted gate verdict,
synthesize a minimal gate section from that verdict so the PR ALWAYS gets a summary
(deep results still fold in below). Hard-throw only when there is genuinely nothing —
no phase content AND no trusted verdict (a local/manual misconfiguration).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…drops a /review The trigger-review job did a FULL clone of the large dotnet/maui repo with only the job's 10-min timeout to stop it. A transient network stall on that clone made the "Checkout repository scripts" step hang until the JOB timeout CANCELLED the whole job — silently, because a cancellation skips the downstream steps (no pipeline queued, no notice). Observed repeatedly: #35640, and #36828 (trigger run 30647459829 cancelled at "Checkout repository scripts"). - Shallow the checkout (fetch-depth: 1) — this job only needs .github/scripts, so it now completes in seconds instead of cloning full history. - Add a per-step timeout-minutes: 4 (< the job's 10) so a rare stall becomes a proper step FAILURE that runs the cleanup/notice steps, instead of a job-level cancellation. - Add a cancelled()/failure() notice step that posts a "transient infra — re-comment /review" warning using gh + the event payload directly (no repo scripts, which may be exactly what failed to check out), so a checkout hang is never silent again. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…eview The AnalyzeCopilotTokenUsage stage is pure telemetry, but it could mark the whole build failed when a run produced no token-usage data: on a SKIPPED-gate run the Copilot agent consumed no tokens, so no copilot-token-usage directory was created, and PublishPipelineArtifact failed with "Path does not exist" — reddening a build whose review had already posted its summary fine. (build 14834473, PR #36984.) - Always create the output dir up front so the publish never hits a missing path. - Mark both the Aggregate step and the Publish task continueOnError:true so a missing artifact or transient publish error can't fail the build. The review stages (ReviewPR / RunDeepUITests / UpdateAISummaryComment) remain the real signal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…(ARG_MAX) At high deep-test failure counts the UI-failure triage prompt was passed to copilot as a single -p argument that exceeded the OS per-argument limit (Linux MAX_ARG_STRLEN 128KB; macOS argv+env). Build 14842388 (#36821, 311 catalyst failures) hit 'Argument list too long' -> analysis silently omitted -> summary fell back to a raw ~1800-line TRX dump with no grouped ✗/●/ℹ triage. - Analyze-UITestFailures.ps1: hard-cap the untrusted analysis input ($maxInputChars = 80000) before building the prompt, with a truncation notice. Guarantees the -p argument stays under the per-arg limit regardless of failure count; the grouped counts still reflect the full run (prep step states true totals). - Prepare-UITestFailureAnalysis.ps1: bound the bulky failures section (per-category sample 25->8, error 12->6, stack 8->4 lines) so the changed-files list and diff always survive within the cap. A representative sample is sufficient for the root-cause grouping the prompt requests. - Add Analyze-UITestFailures.Tests.ps1 pinning the cap invariant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…o space left on device' Build 14843877 (#36767 android) red-failed the ReviewPR/gate stage at 'Build MSBuild Tasks' with 'No space left on device' — pure agent infrastructure, not the PR. The existing 'Free Disk Space for Emulator' cleanup reclaims the same hosted-tool caches but runs LATER (after this build, right before the emulator), so it could not prevent the build from exhausting the disk. Add an android-only pre-build cleanup step (mirrors the proven emulator cleanup) right after 'Verify .dotnet' and before 'Build MSBuild Tasks', reclaiming the unused Linux hosted-image caches (/usr/share/dotnet, hostedtoolcache CodeQL/go/ Python, swift, powershell, chromium) up front. Best-effort + non-blocking so cleanup can never itself fail the review. The build uses ./.dotnet, never the system /usr/share/dotnet (already proven safe by the later cleanup). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…stop hitting NETSDK1147 Root-caused the systemic android NETSDK1147 gate-infra flake (build 14843829 / #36572, and many prior). Sequence: 'Install .NET and workloads' installs the android/ios/maccatalyst workload into ./.dotnet; then 'Build MSBuild Tasks' hits the corrupt-SDK failure (unloadable System.IO.Pipes.dll, exit 134) and, on its retry, WIPES ./.dotnet to reinstall a clean SDK. That wipe also removes the platform WORKLOAD, and the buildtasks reinstall (dotnet-buildtasks) restores only the SDK — so the gate's later test build fails with NETSDK1147 ('the following workloads must be installed: android'). That is pure GATE INFRASTRUCTURE, not the PR, but it surfaced as an ENV/INCONCLUSIVE gate. Fix (targeted, band-correct, idempotent): - When the buildtasks step wipes ./.dotnet, drop a PERSISTENT marker (dotnet-wiped-needs-workloads) that — unlike $wipeMarker — is not cleared on success. - Add a 'Restore .NET workloads if .dotnet was wiped' step right after Build MSBuild Tasks that, ONLY when the marker is present, re-runs the same proven './build.ps1 --target=dotnet' workload install (repo's pinned manifest, never a mismatched band) and clears the marker. The common no-wipe path pays nothing. Also commit 9cd0ebe: free disk space BEFORE the build (android) so the gate build stops hitting 'No space left on device' (#36767) — the other our-infra ENV gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…timeout as ENV, not a FAILED Build 14844563 (#35640 android) produced a FALSE FAILED: all 17 Material3CarouselViewFeatureTests failed identically with 'OneTimeSetUp: System.TimeoutException : Timed out waiting for Go To Test button to appear (the app did not recover after crash-recovery attempts)'. The app under test crashed on launch and never recovered, so EVERY test failed at fixture setup before a single assertion ran — no test actually verified anything. The agent had also just flaked the emulator boot (240s x2), confirming infra. The gate read 'without fix=FAIL, with fix=FAIL' and concluded 'fix does not pass the tests → FAILED', when the honest verdict is INCONCLUSIVE (the app never launched). The existing OneTimeSetUp env-patterns (OpenQA.Selenium, 'Call InitialSetup', 'app could not be found') did not cover the crash-recovery-timeout phrasing, so the failure fell through to a plain FAIL. Fix: add an env-pattern matching 'the app did not recover after crash-recovery attempts' to $envErrorPatterns. This loop is only reached when Passed=0 (the whole fixture failed), so it can never mask a partial real failure — if any test had launched+passed the counts are trusted instead. +2 Pester tests (the false-FAILED case and the partial-pass guard); 33 pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…-flake 'just retry' wording When the gate reaches INCONCLUSIVE with an APP_CRASH (the app under test SIGABRT'd/ exited mid-run), the summary told the author it is 'infrastructure, not a problem with your PR — comment /review to retry on a fresh agent'. That is misleading for a DETERMINISTIC crash: the gate already retries env errors up to 3x (rebooting the device each time), so an APP_CRASH that still reaches INCONCLUSIVE has persisted across every attempt, and a plain retry is unlikely to change it. Proven on #36572 build 14846070: the with-fix MediaPicker ProcessImage test SIGABRT'd on a FRESH agent on all 3 attempts (non-stale logcat 12:35/12:46), while other android runs this session ran fine — a real, reproducible native crash, not a flake. Split the env-error classification: an APP_CRASH excerpt now gets a distinct, non-accusatory message ('the app crashed and it persisted across every attempt; a plain /review retry is unlikely to help; the crash is in the runtime/native libs the test exercises OR the code under test; download the adb-logcat/bugreport to see the native stack'). Non-crash env errors (emulator/sim boot, Appium, empty result file) keep the transient-flake 'retry on a fresh agent' wording. Verdict is UNCHANGED (still INCONCLUSIVE) — this only makes the explanation honest. +1 Pester test (34 pass). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
Brand-new VerifyScreenshot tests fail with 'Baseline snapshot not yet created' because no baseline is committed yet. These are deterministic new-baseline results, not emulator flake. The Android per-test retry was re-running them anyway — they can never pass without a committed baseline, so each wasted a full re-run and could exhaust the deep category time budget on snapshot-heavy test PRs (e.g. #36939, 322 new-baseline 'failures'). It also logged them as 'real failures'. Exclude baseline-not-created failures from the flaky-retry set; if only new-baseline failures remain, skip the Android retry entirely. Downstream ci-copilot.yml already reclassifies them as 'new baseline'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
The Stage 3 deferred inline-review post was gated on the presence of
inline-findings.post.ok. That sentinel is written by the Review stage's
'Task 4: Post' step, which runs AFTER the CopilotLogs artifact is published
and is NOT included in CopilotPostLogs' PRAgent payload — so it never lands
in any artifact Stage 3 restores. The gate was therefore always false and
ALL inline comments were skipped ('No inline sentinel/findings — skipping'),
even though inline-findings.json was produced and round-tripped fine.
Derive the 'PR fix is the winner' signal from winner.json (isPRFix), which
round-trips reliably via CopilotLogs into $targetDir alongside
inline-findings.json — mirroring Review-PR.ps1's own
$isPRWinner = (-not $winner) -or ($winner.isPRFix -eq $true). The legacy
sentinel is still honored as a fast path when present, but its absence no
longer blocks. A non-PR try-fix winner (isPRFix=false) still correctly
suppresses the PR's inline findings.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
A VerifyScreenshot failure 'Snapshot different than baseline: X.png (size differs - baseline is WxH pixels, actual is WxH pixels)' means the committed baseline PNG has different DIMENSIONS than the gate simulator's screenshot — the baseline was captured on a different-sized device than the gate boots (e.g. an iPhone 16 Pro 1206x2472 baseline vs the pinned iPhone 11 Pro 1124x2286). A PR *code* fix can never change screenshot dimensions, so the same mismatch appears in BOTH the without-fix and with-fix runs and the gate cannot A/B verify the test — it must report INCONCLUSIVE, never FAILED. The existing guards only covered 'Baseline snapshot not yet created' and 'N% difference' residuals; a size mismatch has no percentage, so it fell through to a genuine FAILED (build 14850018, PR #37032 iOS Issue36422). Classify it as env/INCONCLUSIVE in both Get-TestResultFromOutput paths (the device-test Passed:/Failed: count path and the UITest NUnit fall-through), guarded so it only fires when every remaining failure is a size mismatch (or size + new-baseline) — a real 'N% difference' pixel diff against a same-size baseline is still counted as a genuine visual regression. +3 Pester tests (37 pass). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…the other leg A missing native shared library (libSkiaSharp/libHarfBuzzSharp .so) on the gate agent crashes the test host before the fixed code runs, so the fix is unverifiable via that test. The existing guard only excluded native-lib failures present in BOTH the without-fix AND with-fix runs ($bothNativeLib). It missed the case where the without-fix leg failed for a DIFFERENT reason — most commonly a compile-coupled build error (new API + test in the same project, so the reverted baseline never reached the native-lib load) — leaving the with-fix libSkiaSharp DllNotFound counted as a genuine FAILED. Reclassify any with-fix NativeLibLoadFailure as env/INCONCLUSIVE in the aggregation loop (mark $w.EnvError), independent of the without-fix leg. A genuine assertion regression never presents as a DllNotFoundException, so this cannot mask a real failure. (build 14850956, PR #35710: GenerateSplash* libSkiaSharp DllNotFound on the Linux android gate, without-fix compile-coupled Passed=False/Failed=0 → wrongly FAILED.) +1 Pester test (38 pass). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
The Post AI Summary stage failed with HTTP 422 'Body is too long (maximum is 65536 characters)' when the review body exceeded GitHub's limit (observed on build 14857215, PR #36884). Both the issue-comment and PR-review POST paths hit the same cap, so both the initial REQUEST_CHANGES and the COMMENT retry were rejected and the whole stage threw. Cap $commentBody at 65500 chars right after it is built (the single chokepoint feeding both POST paths): close any dangling fenced code block so the markdown stays valid, then append a clear truncation notice pointing to the full deep-test detail in the build artifacts. The leading marker (used for stale-comment detection) is preserved because only the tail is trimmed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
… for try-fix Bump the reviewer/judge model from gpt-5.5 to gpt-5.6-sol and run it at the longest context tier with maximum reasoning effort by passing the copilot CLI's --context long_context and --effort max flags. Both the main review (Review-PR.ps1's Invoke-CopilotStep) and the deep UI-failure analysis (Analyze-UITestFailures.ps1) share the new defaults, all env-overridable via COPILOT_REVIEW_MODEL / COPILOT_REVIEW_CONTEXT / COPILOT_REVIEW_EFFORT. Give Invoke-CopilotStep an optional -Model override and use it to run the try-fix candidate-generation step (STEP 5a) on a different model family — claude-opus-4.6 — so the alternative fixes bring genuine model diversity instead of echoing the gpt-5.6-sol reviewer's own reasoning. Overridable via COPILOT_TRYFIX_MODEL. Parse-checked; Review-PR (24) and token-usage (2) Pester suites pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
Ports the review-process half of the model-policy refresh into the reviewer branch (the gh-aw workflow model updates are intentionally NOT included here — they live in the separate CI-workflow PR): - Main PR reviewer + local test-fail reviewer (Review-PR.ps1, Review-Tests.ps1, Analyze-UITestFailures.ps1): GPT-5.6 Sol, invoked with --context long_context --effort max. - Try-fix panel (pr-review skill): Claude Opus 5, Claude Sonnet 5, GPT-5.3-Codex, GPT-5.6 Sol — Opus 5 is one of four attempts, selected per-attempt by the skill (replaces the earlier single -Model override). - MAUI expert reviewer (code-review skill): Claude Opus 5, kept on a different family from the Sol orchestrator to reduce correlated misses. - Vally eval specs: GPT-5.6 Sol executors judged cross-vendor by Opus 5. GPT-5.6 Sol is the majority working model; Opus 5 appears only as the expert reviewer, one try-fix attempt, and cross-vendor eval judge. Reviewer Pester suite: 26 passed. Scripts parse-check clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Failure
GitHub Actions
Failure
github-actions
Bot
added
the
s/agent-review-in-progress
| $model = if ($env:COPILOT_REVIEW_TESTS_MODEL) { $env:COPILOT_REVIEW_TESTS_MODEL } else { "gpt-5.6-sol" } | ||
| Write-Host "Invoking Copilot CLI with model $model..." |
There was a problem hiding this comment.
Fixed in 1b4838d. The local test-failure reviewer is now hard-pinned to gpt-5.6-sol, and regression tests reject COPILOT_REVIEW_TESTS_MODEL overrides.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
MauiBot
removed
the
s/agent-review-in-progress
This comment has been minimized.
This comment has been minimized.
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@kubaflo — new AI review results are available based on commit
1e0a636.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ⚠️ SKIPPED
No tests were detected in this PR.
Recommendation: Add tests to verify the fix using the write-tests-agent.
📋 Pre-Flight — Context & Validation
PR #38040 Pre-Flight
PR: #38040 - Harden maui-copilot reviewer and skill validation with GPT-only models
Author: kubaflo
Base / head: main / improved-reviewer (1e0a636d594127b6fbba81e85f16c28c93952201 from public PR metadata)
Local review commit: 1532097d318ee668d36c9def4f3d8c5f695a2688 (PR #38040 squashed for review)
Issue: None. The PR explicitly describes accumulated reviewer-infrastructure hardening rather than one linked product issue.
Requested platform: Android
Scope and Diff
The local origin/main..HEAD diff was inspected directly. It contains 94 files, 23,761 additions, and 1,366 deletions. A path-based pre-flight classification found 49 implementation/infrastructure files and 45 test or fixture files. Most changes are reviewer infrastructure: 50 files under .github/scripts, 24 under .github/skills, nine under eng, five workflows, and two test-infrastructure files under src.
The submitted implementation combines several mechanisms:
- deterministic Gate classification and reruns, including
SKIPPEDfor test-less PRs and better baseline/fix failure attribution; - runnable-test detection that excludes deleted and helper-only C# files and derives narrower filters from changed methods;
- fixed GPT model selection, with the main reviewer pinned to
gpt-5.6-soland exactly two ordered try-fix models; - wall-clock watchdogs and deep-stage budgets intended to preserve cleanup and publication time;
- immutable reviewed-head handoff plus bounded, canonical, reparse-point-safe artifact import into credentialed posting;
- hermetic Vally fixture/runtime preparation and fail-closed model/evaluation validation;
- serialized recovery of missed
/reviewcomments without prematurely acknowledging them; - Android and Apple hosted-agent/device setup hardening.
Existing Evidence and Feedback
- The supplied Gate result is SKIPPED — no tests detected. It must not be rerun.
- No primary test was detected by the Gate. The request did not include an additional mandatory-regression command, so no unrequested broad suite may be substituted.
- The PR description reports prior local and live-Azure evidence, including a 2,165-test PowerShell workflow run, focused reviewer/model-policy suites, Ruby fixture tests, YAML parsing, and real PR Gate runs. These are reported evidence, not results reproduced in this step.
- Earlier review feedback found a narrower final
inflight/[a-z]+snapshot validator and a free-form local test-review model override. Public discussion says both were corrected on later commits, and the current PR description records the corrected numeric/dotted inflight validation and hard-pinnedgpt-5.6-soltest reviewer. - Earlier try-fix attempts were non-implementing and blocked: (1) diff-status/hunk-derived runnable-test detection and (2) an absolute publication-safe child-process deadline. New attempts must not repeat either mechanism.
- Repeated prior review runs produced incomplete-review notices before a later AI summary. This supports the PR's timeout/always-finish objective but is not a candidate test result.
Try-Fix Inputs and Boundaries
Problem: Find one mechanism-level alternative to the submitted reviewer hardening that addresses a concrete reliability or security failure without relaxing the GPT-only policy, immutable snapshot binding, credential separation, bounded artifact import, or durable recovery semantics.
Target files: Existing modified infrastructure files in origin/main..HEAD; files outside the baseline state's RevertedFiles are read-only.
Test command: No primary test was detected and no mandatory regression command was supplied. An attempt must report Blocked rather than inventing or running a broad suite. If the attempt reaches a safe implementation path and identifies an already-existing focused regression command, it may run only that focused command.
Known restoration blockers: The worktree contains pre-existing unrelated tracked edits that must not be changed or discarded. In addition, the PR adds three non-.github implementation scripts:
eng/scripts/dismiss-apple-account-dialog.sheng/scripts/dismiss-maccatalyst-app-recovery-dialog.sheng/scripts/run-as-console-user.sh
EstablishBrokenBaseline.ps1 requires a clean tracked worktree, and the try-fix skill forbids editing when baseline NewFiles is non-empty. Each attempt must run the mandated baseline command once, preserve all pre-existing changes, record an honest Blocked result if either boundary applies, and run the exact restore command before returning.
Prior Approach Constraints
Do not repeat:
- the PR's combined implementation mechanisms listed above;
- the earlier diff-status/hunk-derived runnable-test detection candidate;
- the earlier absolute publication-safe child-process deadline candidate;
- the already-corrected inflight-validator synchronization or local test-review model pinning fixes.
The dedicated expert review is deferred to STEP 5b. No separate expert-review agent was launched during this pre-flight.
🔬 Code Review — Deep Analysis
Expert PR Evaluation
Verdict: NEEDS_DISCUSSION
Confidence: low
Independent assessment
The submitted PR hardens the MAUI Copilot review pipeline across trusted/untrusted execution boundaries, immutable review-head binding, Gate classification and retry behavior, bounded process and artifact handling, missed-command recovery, UI-test host preparation, and GPT-only reviewer/evaluator model selection. The approach is security-conscious and backed by focused script-level regression coverage throughout the changed infrastructure.
The single maui-expert-reviewer pass found no actionable file:line defects on the current submitted diff, so inline-findings.json is an empty JSON array.
Prior review reconciliation
All three public review surfaces were queried. No current inline or top-level ❌ error finding remains unresolved.
An earlier MauiBot review of commit 1b4838d29c9b5b0b958b5d5c086eef9a8adb36e8 found that .github/scripts/Review-PR.ps1 accepted only alphabetic inflight/* suffixes while the pipeline accepted safe numeric/dotted suffixes. The current submitted HEAD resolves that finding at .github/scripts/Review-PR.ps1:672 by using inflight/[a-z0-9][a-z0-9._-]*, matching the other trusted validators. The previous finding is therefore fixed, not carried forward.
Blast radius
- Runs for all review invocations: many changes affect shared Setup, Gate, CopilotReview, deep-test, and Post paths rather than an opt-in feature.
- Startup impact: yes; branch/snapshot validation, trusted overlays, model policy, and artifact import execute before or between review phases.
- Static/shared state: pipeline artifacts, sentinels, immutable snapshot metadata, and hosted-agent state cross task/job boundaries.
This is broad, credential-adjacent CI infrastructure, so confidence cannot exceed low without clean execution evidence.
CI and Gate evidence
- Trusted Gate:
SKIPPED — no tests detected; the supplied Gate result is authoritative and was not rerun. - Current public checks: several
maui-prjobs remain in progress, while Skill Validation and multiple evaluator/hermeticity jobs report failure. Attribution was not established before this required evaluation. - Classification: CI safety is undetermined. Under the review contract, pending/red or absent required-check evidence precludes
LGTMeven though the code-only expert pass found no defect.
Failure-mode probing
- Valid numeric/dotted inflight target: the current Setup validator accepts names such as
inflight/10.0.101, consistent with the pipeline validators, so the older false rejection no longer occurs. - Unsafe inflight target: suffixes beginning with punctuation, containing uppercase characters, nested slashes, or traversal characters still fail the anchored allow-list.
- Prompt-controlled artifact ambiguity: the submitted import logic requires the canonical contained PRAgent tree, bounds traversal and file sizes, rejects reparse-point ambiguity, and stops when a second candidate proves ambiguity.
- Ordinary child-process failure versus timeout: the bounded execution paths retain exit-code/log evidence rather than treating every nonzero exit as a hang; watchdog termination is tracked separately.
External Output Contract
| Consumer token/pattern | Producer location | Producer emission condition | Consumer assumption | Ordinary negative case | Downstream effect |
|---|---|---|---|---|---|
base.ref matched by ^(main|net...|inflight/[a-z0-9][a-z0-9._-]*|release/...)$ |
GitHub Pull Request REST metadata consumed by .github/scripts/Review-PR.ps1 |
GitHub emits the exact target branch name for every PR | Only supported, path-safe target branches may be persisted into the immutable snapshot | A legitimate target such as inflight/10.0.101 |
Accepted at current HEAD; malformed or unsupported names fail before snapshot publication |
Verdict: NEEDS_DISCUSSION
Confidence: low
Summary: The current submitted code has no expert-review finding, and the sole previously reported correctness error is fixed at HEAD. Approval is nevertheless not justified by the supplied evidence because the trusted Gate skipped and current public CI is still pending/red; candidate comparison must preserve that uncertainty.
🛠️ Try-Fix — Analysis & Comparison
PR #38040 Try-Fix Aggregate
Bound: At most two sequential candidates, one pass plus at most one correction each.
Gate: SKIPPED — no tests detected; Gate verification was not rerun.
Platform: Android
Candidate 1 — PR-controlled output-volume circuit breaker
Model: gpt-5.3-codex
Result: Blocked
Status: Done
Files changed: None
Self-review: 0 findings
Approach
Add a byte-budget circuit breaker around PR-controlled subprocess output in trusted reviewer scripts. The trusted wrapper would count streamed stdout/stderr bytes, terminate the child process when the budget is exceeded, and classify the run as INCONCLUSIVE with an explicit over-budget reason.
Prior approaches avoided
This does not repeat the submitted PR's deterministic Gate/rerun and runnable-test detection changes, fixed GPT model policy, wall-clock watchdog/deep-stage budgets, immutable snapshot and bounded canonical artifact import, hermetic Vally preparation, serialized recovery, the earlier diff-status/hunk-derived runnable-test candidate, the earlier absolute publication-safe deadline candidate, or the corrected inflight-validator/model-override fixes.
Mechanism-level difference
The existing controls bound elapsed time, candidate eligibility, and persisted artifacts. This candidate instead targets live output-flood resource exhaustion: a PR-controlled child can emit enough stdout/stderr to exhaust agent memory, log limits, or intermediate storage before a time watchdog or post-run artifact cap helps. A streaming byte counter would stop that cause at the subprocess boundary.
Diff
No diff was produced.
Test result and failure analysis
The mandatory baseline command failed because the tracked worktree contains pre-existing unrelated edits. No baseline state was created, and those edits were correctly left untouched. The caller supplied no authorized primary or mandatory regression command after the Gate reported no detected tests, so no substitute test was run. The candidate therefore could not be legally implemented or empirically tested.
The exact restore command ran and returned No baseline state found / Restored=False, which is the expected no-state result after a baseline failure before any attempt edit.
Detailed record: ../try-fix-1/content.md
Candidate 2 — Ownership-scoped process cleanup
Model: gpt-5.6-sol
Result: Blocked
Status: Done
Files changed: None
Self-review: 0 findings
Approach
Replace Review-PR.ps1's global node/copilot process sweep with cleanup limited to process identities started by the current review invocation and descendants whose ownership chain is validated before termination.
Prior approaches avoided
This differs from all submitted mechanisms and earlier alternatives listed for Candidate 1, and it does not revisit Candidate 1's live output-volume circuit breaker. It targets process ownership rather than test classification, model selection, elapsed-time or byte budgets, artifact integrity, fixture isolation, recovery serialization, or branch validation.
Mechanism-level difference
The current name/path-based sweep can match healthy node or copilot processes owned by concurrent reviews on the same host. Recording the current invocation's child identities and validating each descendant's parent chain before teardown would remove only leaked processes causally owned by this run, preserving unrelated review processes even when their names and executable paths match.
Diff
No diff was produced.
Test result and failure analysis
The mandatory baseline command again rejected the pre-existing dirty tracked worktree, so no baseline state or RevertedFiles edit allow-list was created. The caller supplied no authorized primary or mandatory regression command after the Gate reported no detected tests; no substitute test was run. The candidate therefore remained a distinct design hypothesis rather than a legal implementation.
The exact restore command ran and returned No baseline state found / Restored=False, the expected no-state result because no attempt edit occurred.
Detailed record: ../try-fix-2/content.md
Aggregate Outcome
Exactly two ordered candidates were attempted. Both identified mechanism-distinct alternatives and both were blocked before implementation by the mandatory baseline boundary; neither produced a diff or an empirical test result. Candidate 2 consumed Candidate 1's persisted result and did not repeat its approach. No Gate verification, full test suite, cross-pollination, expert-review child agent, or follow-up audit was run.
🏁 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Winner: pr
The submitted PR is the strongest candidate. The sole expert pass found no actionable defect at the current HEAD, and the blocking inflight-validator mismatch reported against an older commit is now fixed. pr-plus-reviewer has an empty reviewer diff and is therefore behaviorally equivalent, while both STEP 5a alternatives remained unimplemented design hypotheses.
The raw PR cannot receive an approval recommendation from this run: the trusted Gate was SKIPPED — no tests detected, and current public checks include pending maui-pr work plus failures in Skill Validation/evaluator jobs. Their attribution remains uncertain, so the evidence does not establish merge readiness.
Comparative ranking
| Rank | Candidate | Implemented candidate delta | Regression evidence | Assessment |
|---|---|---|---|---|
| 1 | pr |
Submitted PR | Trusted Gate SKIPPED; public CI pending/red | Best complete implementation. Current expert review found no actionable code defect, prior blocking feedback is fixed, and the PR contains extensive focused test infrastructure. Wins on implementation completeness, not on clean validation evidence. |
| 2 | pr-plus-reviewer |
No; reviewer.patch is empty |
Not run because there was no reviewer change and no required targeted command | Equivalent to pr, but adds no improvement. Its pre-created trusted overlay is validation infrastructure and does not constitute a reviewer-authored candidate fix. |
| 3 | try-fix-2 |
No; blocked before editing | Blocked, no test run | Ownership-scoped process cleanup identifies a plausible concurrency hazard in global node/copilot sweeping, but it has no diff or empirical evidence and does not displace the completed PR. |
| 4 | try-fix-1 |
No; blocked before editing | Blocked, no test run | A live output-volume circuit breaker is mechanism-distinct and potentially useful, but it likewise has no implementation or test result and is broader than a required correction to this PR. |
No candidate failed an executed regression test, and no candidate passed one during STEP 5a/5b. The passed-versus-failed ordering rule therefore does not alter the ranking; blocked/no-op candidates rank below the complete submitted implementation.
Expert review reconciliation
inline-findings.jsoncontains[].- The earlier
.github/scripts/Review-PR.ps1error for numeric/dottedinflight/*targets applied to commit1b4838d29c9b5b0b958b5d5c086eef9a8adb36e8. Current HEAD usesinflight/[a-z0-9][a-z0-9._-]*at the final immutable-snapshot boundary, so that finding is fixed. - No current top-level, inline, or issue-comment
❌finding was confirmed as unresolved.
Gate, CI, and risk
- Gate:
SKIPPED — no tests detected; it does not permit approval. - Current checks: several build/integration jobs are still in progress, and Skill Validation plus evaluator/hermeticity jobs report failure. Failure attribution was not established in this bounded comparison.
- Blast radius: broad shared, credential-adjacent reviewer infrastructure spanning Setup, Gate, CopilotReview, deep tests, and Post.
- Confidence: low.
Required disposition
Keep the raw PR implementation rather than adopting either speculative try-fix candidate. Resolve the current failing checks and obtain the intended focused validation evidence before merge; no reviewer patch is recommended from this pass.
📱 UI Tests — Button,Label,Layout
Detected UI test categories: Button,Label,Layout
✅ Deep UI tests — 360 passed, 0 failed, 7 skipped across 3 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Button |
71/73 (2 skipped) ✓ | — |
Label |
97/99 (2 skipped) ✓ | — |
Layout |
192/195 (3 skipped) ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
Improves the reliability and security of the
maui-copilotAzDO review pipeline (definition 27723), its GitHub trigger/recovery path, and its skill-evaluation gates, while enforcing a GPT-only model policy across reviewer automation.Issues Fixed
This is infrastructure hardening accumulated from production reviewer failures rather than a fix for one product issue. The real-run examples below identify the PRs whose failures supplied the regression evidence.
Gate reliability fixes
The gate ("test before and after fix") had several classes of false verdicts. Each is addressed:
PASSEDplus "exited before writing report" warningINCONCLUSIVEafter 3 wasted retriesSKIPPED(add tests), no retryINCONCLUSIVEVerifyScreenshottest, no baseline yetFAILED("fix does not pass")INCONCLUSIVE("new snapshot - no baseline")FAILEDFAILEDPASSED- at least one test must reproduce the bug (FAIL to PASS) and none may regressINCONCLUSIVEThe gate now always runs: the temporary
SkipUITestsfast-mode toggle, which passed-SkipGateand skipped the gate on every production review, is removed. The gate self-selects its verdict, so a PR with no runnable tests exitsSKIPPEDwithout emulator work.This also removes the spurious
CopilotFailed: command not foundnoise in the "Check Review Result" step, where undefined$(var)macros were evaluated as Bash command substitutions.Deep-stage and always-finish reliability
[Process]::StartplusKill($true)) bounds the buildtasks/cake child-process tree because AzDO's steptimeoutInMinutesdoes not kill a hung child tree.ENOSPCretries on an already-full agent.PRState/<PR>/PRAgenttree. Candidate discovery uses bounded iterative traversal, skips reparse-point directories, validates containment, caps the directory count, and stops as soon as a second candidate proves ambiguity.inflight/*names such asinflight/10.0.101while rejecting hidden, uppercase, nested, and traversal-like shapes.Skill-evaluation and trigger hardening
GPT-only model policy
gpt-5.6-sol;COPILOT_REVIEW_MODELcan no longer redirect it to an arbitrary provider.gpt-5.3-codex, thengpt-5.6-sol.gpt-5.3-codex.gpt-5.6-sol;COPILOT_REVIEW_TESTS_MODELcan no longer redirect it.gpt-5.6-solorgpt-5.3-codex.gpt-5.3-codex.Files changed
.github/workflows/review-trigger.yml-/reviewcomment parsing for platform and branch arguments.eng/pipelines/ci-copilot.yml- gate/provisioning behavior, watchdogs, deferred posting, snapshot handling, artifact isolation, and base validation..github/scripts/Review-PR.ps1- gate classification, bounded review artifacts, pinned primary reviewer, and GPT-only try-fix candidates..github/scripts/shared/Detect-TestsInDiff.ps1- skips C# files with no test-method attributes..github/scripts/shared/Import-ExpectedPRAgentArtifact.ps1- bounded, reparse-safe canonical artifact discovery..github/scripts/shared/Analyze-UITestFailures.ps1- approved GPT model allowlist..github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1- deterministic re-runs and complete report/classification behavior..github/skills/pr-review/SKILL.mdand.github/skills/code-review/SKILL.md- GPT-only try-fix and expert-review selection..github/scripts/PrepareVallyEvaluation.rb,.github/scripts/SetupVallyRuntime.sh, and.github/workflows/skill-validation.yml- hermetic fixture preparation, runtime isolation, and fail-closed evaluation gates..github/skills/**/tests/*.vally.yamland model-probe workflows - GPT-only evaluation judges and probes..github/scripts/CopilotPipeline.Tests.ps1- synchronized branch-validation and forbidden-model regression coverage..github/scripts/post-ai-summary-comment.ps1- AI Review Summary rendering.What NOT to do
/reviewcomment in the scheduled scanner before the serialized trigger workflow runs. The durable marker is the trigger workflow's completion/dedupe signal; pre-marking would make that dispatched run skip itself.Testing
Validated with full-gate runs (
SkipUITests=false) on real PRs:SKIPPED(was falseINCONCLUSIVE)PASSEDwith a report written (wasPASSEDplus a warning)INCONCLUSIVEwith "Pre-existing build failure" headline (was mislabeled "Fix does not compile")inflight/10.0.101base accepted and resolved to the expected base SHALocal model-policy validation:
inflight/10.0.101, while the corrected validator passes.c34f9c8ee90.Live Azure evidence:
15156195succeeded on1e0a636d594127b6fbba81e85f16c28c93952201.System.IO.Pipesmissing); the existing retry wiped./.dotnet, reinstalled cleanly, and both build-tasks builds succeeded.SKIPPEDbecause this infrastructure-only change exposed no runnable product test.gpt-5.6-solandgpt-5.3-codex. The persisted try-fix candidates were exactlygpt-5.3-codex, thengpt-5.6-sol; the raw Task 3 log contained noclaudeoranthropictext.inline-findings.jsonwas empty, and the submitted PR won the candidate comparison.Button,Label, andLayout.1e0a636, stale artifact cleanup, and review-lock removal all succeeded.