diff --git a/.github/pr-review/pr-preflight.md b/.github/pr-review/pr-preflight.md index 0c5ae3f9b0b3..f27631794756 100644 --- a/.github/pr-review/pr-preflight.md +++ b/.github/pr-review/pr-preflight.md @@ -1,10 +1,10 @@ -# PR Pre-Flight — Context Gathering +# PR Pre-Flight — Context Gathering & Code Review -> **SCOPE:** Document only. No code analysis. No fix opinions. No running tests. +> **SCOPE:** Gather context, classify files, and perform deep code review. No code changes. No fix selection. No test execution. --- -## Steps +## Part A: Context Gathering (Steps 1–6) 1. **Read the issue** — full body + ALL comments via GitHub MCP tools 2. **Find the PR** — read description, diff summary, review comments, inline feedback @@ -35,7 +35,58 @@ gh pr view XXXXX --json comments --jq '.comments[] | select(.body | contains("Fi --- -## Output File +## Part B: Code Review (Step 7) + +> **Purpose:** Perform deep code analysis using the `code-review` skill to surface correctness issues, safety concerns, and MAUI convention violations BEFORE Try-Fix explores alternatives. These findings guide Try-Fix models toward higher-quality fixes. + +> **🚨 Independence-first requirement:** Step 7 MUST be invoked as a **separate sub-agent** (via the `task` tool with `agent_type: "general-purpose"`) so the code-review skill can form its assessment from the code BEFORE reading any PR narrative. The sub-agent receives ONLY the PR number — not the context gathered in Part A. This prevents anchoring bias. +> +> **Validation constraint:** The Step 7 prompt MUST NOT contain issue titles, root-cause descriptions, bug summaries, or any Part A content — only `PR #XXXXX`. If you find yourself adding context "to help" the sub-agent, you are violating independence-first. + +7. **Invoke the code-review skill as a sub-agent:** + + Use the `task` tool to launch a separate agent. The prompt MUST NOT contain issue titles, root-cause descriptions, or any Part A context — only the PR number. + + ```python + task( + name="code-review", + description="Code review for PR", + agent_type="general-purpose", + mode="sync", + prompt=""" + Run the code-review skill for PR #XXXXX. + Follow the full 6-step workflow in .github/skills/code-review/SKILL.md. + Output the review in the format specified by that skill. + """ + ) + ``` + + The sub-agent internally follows the code-review skill's 6-step workflow: + 1. Gather code context (independence-first — reads code BEFORE PR description) + 2. Load MAUI review rules from `.github/skills/code-review/references/review-rules.md` + 3. Form independent assessment + 4. Reconcile with PR narrative and prior reviews + 5. Check CI status + 6. Blast radius, failure-mode probing, and verdict + +**If Step 7 fails, times out, or returns malformed output:** +- Write `pre-flight/code-review.md` with: `## Code Review: SKIPPED\n\nReason: {failure description}` +- Set verdict to `SKIPPED` in the Code Review Summary section of `content.md` +- Omit `hints` from Try-Fix prompts (the `hints` field becomes optional when code review is unavailable) +- Do NOT apply the code-review hard gate in Phase 3 (Report) — treat as if code review was not run + +**Store the sub-agent's full output** in `pre-flight/code-review.md` — use the exact output format from the code-review skill (do NOT reformat or summarize into a different template). + +**Extract key items for Try-Fix consumption** and add to `content.md`: +- All ❌ Error findings (with file:line references) +- All ⚠️ Warning findings (with file:line references) +- Failure-mode probes and their answers +- Blast radius assessment summary +- The overall verdict and confidence level + +--- + +## Output Files ```bash mkdir -p CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/pre-flight @@ -52,16 +103,29 @@ Write `content.md`: - {Finding 1} - {Finding 2} +### Code Review Summary +**Verdict:** {LGTM / NEEDS_CHANGES / NEEDS_DISCUSSION / SKIPPED} +**Confidence:** {high / medium / low / N/A} +**Errors:** {count} | **Warnings:** {count} | **Suggestions:** {count} + +Key code review findings: +- {❌/⚠️/💡} {Brief finding with file:line reference} +- ... +*(If SKIPPED: "Code review sub-agent failed or timed out. Reason: {details}")* + ### Fix Candidates | # | Source | Approach | Test Result | Files Changed | Notes | |---|--------|----------|-------------|---------------|-------| | PR | PR #XXXXX | {approach} | ⏳ PENDING (Gate) | `file.cs` | Original PR | ``` +Write `code-review.md` — the exact output from the code-review sub-agent, in the format specified by `.github/skills/code-review/SKILL.md` (Review Output Format section). Do NOT reformat or create a custom template — preserve the skill's native output verbatim. + --- ## Common Mistakes -- ❌ Researching root cause — save for Try-Fix phase -- ❌ Looking at implementation code — just gather context +- ❌ Skipping the code-review step — it provides critical findings for Try-Fix +- ❌ Reading the PR description before code in Step 7 — independence-first prevents anchoring bias - ❌ Running tests — that's the Gate phase +- ❌ Proposing fixes — save fix ideas for Try-Fix phase diff --git a/.github/pr-review/pr-report.md b/.github/pr-review/pr-report.md index 89651509fd3f..b715ba365327 100644 --- a/.github/pr-review/pr-report.md +++ b/.github/pr-review/pr-report.md @@ -12,18 +12,26 @@ - Phases 1-2 (Pre-Flight, Try-Fix) must be complete before starting - Gate result is available from the prompt (ran separately before this skill) +- **Read `pre-flight/content.md`** to get the code-review summary (verdict, confidence, error/warning counts) +- Optionally read `pre-flight/code-review.md` for full findings if needed for the recommendation --- ## Steps -1. **Determine recommendation:** +1. **Determine recommendation** (rows evaluated in order — first match wins): - | Condition | Recommendation | - |-----------|----------------| - | PR's fix selected and Gate passed | `✅ APPROVE` | - | Alternative fix found via Try-Fix | `⚠️ REQUEST CHANGES` — suggest alternative | - | Gate failed | `⚠️ REQUEST CHANGES` — fix doesn't work | + | Priority | Condition | Recommendation | + |----------|-----------|----------------| + | 1 | Code review verdict is `NEEDS_CHANGES` (any ❌ errors) | `⚠️ REQUEST CHANGES` — code review found errors | + | 2 | Gate failed (tests fail with fix) | `⚠️ REQUEST CHANGES` — fix doesn't work | + | 3 | Alternative fix found via Try-Fix that is simpler/better | `⚠️ REQUEST CHANGES` — suggest alternative | + | 4 | Code review verdict is `NEEDS_DISCUSSION` | `⚠️ REQUEST CHANGES` — include code review concerns | + | 5 | PR's fix selected AND Gate passed AND code review LGTM or SKIPPED | `✅ APPROVE` | + + **🚨 Hard gate:** If the code review (from Pre-Flight) has verdict `NEEDS_CHANGES`, the final recommendation MUST be `REQUEST CHANGES` regardless of Gate or Try-Fix results. Code-review ❌ Errors cannot be overridden by passing tests alone. + + **Code review SKIPPED:** If the code-review sub-agent failed or timed out (verdict = `SKIPPED`), the hard gate does NOT apply. Proceed as if code review was not available — base the recommendation on Gate and Try-Fix results only. Note in the report that code review was unavailable. 2. **Write output files** — Save recommendation to `content.md` @@ -47,10 +55,14 @@ Write `content.md`: | Phase | Status | Notes | |---|---|---| | Pre-Flight | ✅ COMPLETE | {notes} | +| Code Review | {verdict} ({confidence}) | {error_count} errors, {warning_count} warnings | | Gate | ✅ PASSED | {platform} | | Try-Fix | ✅ COMPLETE | {N} attempts, {M} passing | | Report | ✅ COMPLETE | | +### Code Review Impact on Try-Fix +{Brief description of how code-review findings influenced try-fix exploration. Did any model specifically address a code review ❌ Error? Did failure-mode probes reveal issues that guided fix approaches?} + ### Summary {Brief summary of the review} @@ -58,7 +70,7 @@ Write `content.md`: {Root cause analysis} ### Fix Quality -{Assessment of the fix} +{Assessment of the fix — informed by both gate results and code review findings} ``` --- diff --git a/.github/scripts/Review-PR.ps1 b/.github/scripts/Review-PR.ps1 index b4f43982bae3..23937b2bb5af 100644 --- a/.github/scripts/Review-PR.ps1 +++ b/.github/scripts/Review-PR.ps1 @@ -499,8 +499,11 @@ The agent will analyze the issue, determine the appropriate test type (UI test, } } -# Post gate result as a separate PR comment -$postGateScript = Join-Path $PSScriptRoot "post-gate-comment.ps1" +# Post gate result by updating (or creating) the unified AI Summary comment. +# The same script is called again in STEP 3 once the review phases finish; here +# it runs early so the PR author sees the gate outcome without waiting for the +# full review. +$postGateScript = Join-Path $PSScriptRoot "post-ai-summary-comment.ps1" if (Test-Path $postGateScript) { try { if ($DryRun) { @@ -509,10 +512,10 @@ if (Test-Path $postGateScript) { & $postGateScript -PRNumber $PRNumber } } catch { - Write-Host " ⚠️ Failed to post gate comment (non-fatal): $_" -ForegroundColor Yellow + Write-Host " ⚠️ Failed to post gate section (non-fatal): $_" -ForegroundColor Yellow } } else { - Write-Host " ⚠️ post-gate-comment.ps1 not found" -ForegroundColor Yellow + Write-Host " ⚠️ post-ai-summary-comment.ps1 not found" -ForegroundColor Yellow } # Apply gate result label diff --git a/.github/scripts/post-ai-summary-comment.ps1 b/.github/scripts/post-ai-summary-comment.ps1 index b53807632259..2648c71082c5 100644 --- a/.github/scripts/post-ai-summary-comment.ps1 +++ b/.github/scripts/post-ai-summary-comment.ps1 @@ -13,9 +13,16 @@ After posting, the PR author is @-mentioned so they know to review. Content is auto-loaded from PRAgent phase files: + CustomAgentLogsTmp/PRState//PRAgent/gate/content.md (always shown first, open) CustomAgentLogsTmp/PRState//PRAgent/{pre-flight,try-fix,report}/content.md + CustomAgentLogsTmp/PRState//PRAgent/pre-flight/code-review.md - Gate is posted separately by post-gate-comment.ps1. + Gate is included as a section inside this unified comment — the script may + be called by Review-PR.ps1 twice per run: once after the gate completes + (gate-only update) and once after the review phases finish (full update). + + Any standalone legacy "" comment from older versions of + the script is deleted after a successful post to avoid duplicates. .PARAMETER PRNumber The pull request number (required) @@ -60,9 +67,34 @@ if (-not (Test-Path $PRAgentDir)) { } $phases = [ordered]@{ - "pre-flight" = @{ File = "pre-flight/content.md"; Icon = "🔍"; Title = "Pre-Flight — Context & Validation" } - "try-fix" = @{ File = "try-fix/content.md"; Icon = "🔧"; Title = "Fix — Analysis & Comparison" } - "report" = @{ File = "report/content.md"; Icon = "📋"; Title = "Report — Final Recommendation" } + "pre-flight" = @{ File = "pre-flight/content.md"; Icon = "🔍"; Title = "Pre-Flight — Context & Validation" } + "code-review" = @{ File = "pre-flight/code-review.md"; Icon = "🔬"; Title = "Code Review — Deep Analysis" } + "try-fix" = @{ File = "try-fix/content.md"; Icon = "🔧"; Title = "Fix — Analysis & Comparison" } + "report" = @{ File = "report/content.md"; Icon = "📋"; Title = "Report — Final Recommendation" } +} + +# ─── Gate content (rendered first, always open) ─── +$gateSection = $null +$gateFilePath = Join-Path $PRAgentDir "gate/content.md" +if (Test-Path $gateFilePath) { + $gateContent = Get-Content $gateFilePath -Raw -Encoding UTF8 + if (-not [string]::IsNullOrWhiteSpace($gateContent)) { + Write-Host " ✅ gate ($((Get-Item $gateFilePath).Length) bytes)" -ForegroundColor Green + $gateSection = @" +
+🚦 Gate — Test Before & After Fix + +--- + +$gateContent + +
+"@ + } else { + Write-Host " ⏭️ gate (empty)" -ForegroundColor Gray + } +} else { + Write-Host " ⏭️ gate (not found)" -ForegroundColor Gray } $phaseSections = @() @@ -93,8 +125,8 @@ $content } } -if ($phaseSections.Count -eq 0) { - throw "No phase content found. Ensure at least one content.md exists in $PRAgentDir." +if (-not $gateSection -and $phaseSections.Count -eq 0) { + throw "No gate or phase content found. Ensure at least one of gate/content.md or {phase}/content.md exists in $PRAgentDir." } # ============================================================================ @@ -123,7 +155,13 @@ $timestamp = (Get-Date).ToUniversalTime().ToString("yyyy-MM-dd HH:mm UTC") # BUILD NEW SESSION BLOCK # ============================================================================ -$phaseContent = $phaseSections -join "`n`n---`n`n" +# Combine gate (always first, open) with phases (collapsed). When only one +# kind of content is available, the session still renders cleanly. +$sessionParts = @() +if ($gateSection) { $sessionParts += $gateSection } +if ($phaseSections.Count -gt 0) { $sessionParts += ($phaseSections -join "`n`n---`n`n") } +$phaseContent = $sessionParts -join "`n`n---`n`n" + $sessionMarkerStart = "" $sessionMarkerEnd = "" @@ -176,12 +214,17 @@ function Merge-Sessions { foreach ($sha in $orderedKeys) { $block = $sessions[$sha] if ($isFirst) { - # Ensure latest session has
- $block = $block -replace '', '
' + # Ensure ONLY the outer (session-wrapping) details tag is open. Inner + # phase tags must keep their original open/collapsed state — we used + # to re-open all of them via a global regex replace, which forced + # every phase to expand on each new session. + $rx = [regex]::new('') + $block = $rx.Replace($block, '
', 1) $isFirst = $false } else { - # Collapse older sessions - $block = $block -replace '', '
' + # Collapse the outer details of older sessions; leave inner phases alone. + $rx = [regex]::new('') + $block = $rx.Replace($block, '
', 1) } $allSessions += $block } @@ -300,3 +343,25 @@ try { } finally { Remove-Item $tempFile -ErrorAction SilentlyContinue } + +# ============================================================================ +# CLEAN UP LEGACY STANDALONE GATE COMMENTS +# ============================================================================ +# Earlier versions of this workflow posted gate results in a separate comment +# marked with . Now that the gate is included as a section in +# this unified comment, those legacy comments are duplicates and should go. + +try { + $legacyMarker = "" + $allRaw = gh api "repos/dotnet/maui/issues/$PRNumber/comments" --paginate 2>$null + if ($allRaw) { + $allComments = $allRaw | ConvertFrom-Json + $legacy = @($allComments | Where-Object { $_.body -and $_.body.Contains($legacyMarker) }) + foreach ($lc in $legacy) { + Write-Host "🧹 Deleting legacy gate comment (ID: $($lc.id))..." -ForegroundColor Gray + gh api --method DELETE "repos/dotnet/maui/issues/comments/$($lc.id)" 2>&1 | Out-Null + } + } +} catch { + Write-Host "⚠️ Legacy gate-comment cleanup failed (non-fatal): $_" -ForegroundColor Yellow +} diff --git a/.github/scripts/post-gate-comment.ps1 b/.github/scripts/post-gate-comment.ps1 deleted file mode 100644 index fcf365f2f286..000000000000 --- a/.github/scripts/post-gate-comment.ps1 +++ /dev/null @@ -1,271 +0,0 @@ -#!/usr/bin/env pwsh -<# -.SYNOPSIS - Posts or updates the gate verification comment on a GitHub Pull Request. - -.DESCRIPTION - Maintains ONE comment per PR, identified by marker. - Each gate run adds an expandable session keyed by HEAD commit SHA. - - Same commit SHA → replaces that session in-place. - - New commit SHA → prepends a new session (latest first). - Older sessions stay collapsed; the newest is expanded by default. - - After posting, the PR author is @-mentioned so they know to review. - - Reads content from CustomAgentLogsTmp/PRState//PRAgent/gate/content.md. - -.PARAMETER PRNumber - The pull request number (required) - -.PARAMETER DryRun - Print comment instead of posting - -.EXAMPLE - ./post-gate-comment.ps1 -PRNumber 12345 - -.EXAMPLE - ./post-gate-comment.ps1 -PRNumber 12345 -DryRun -#> - -param( - [Parameter(Mandatory = $true)] - [int]$PRNumber, - - [Parameter(Mandatory = $false)] - [switch]$DryRun -) - -$ErrorActionPreference = "Stop" -$MARKER = "" - -# ============================================================================ -# LOAD GATE CONTENT -# ============================================================================ - -$gateContentPath = "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/gate/content.md" -if (-not (Test-Path $gateContentPath)) { - $repoRoot = git rev-parse --show-toplevel 2>$null - if ($repoRoot) { - $gateContentPath = Join-Path $repoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/gate/content.md" - } -} - -if (-not (Test-Path $gateContentPath)) { - Write-Host "⚠️ No gate content found at: $gateContentPath" -ForegroundColor Yellow - exit 0 -} - -$gateContent = Get-Content $gateContentPath -Raw -Encoding UTF8 -if ([string]::IsNullOrWhiteSpace($gateContent)) { - Write-Host "⚠️ Gate content is empty" -ForegroundColor Yellow - exit 0 -} - -Write-Host "✅ Loaded gate content ($($gateContent.Length) chars)" -ForegroundColor Green - -# ============================================================================ -# FETCH PR METADATA (commit + author) -# ============================================================================ - -try { - $commitJson = gh api "repos/dotnet/maui/pulls/$PRNumber/commits" --jq '.[-1] | {message: .commit.message, sha: .sha}' 2>$null | ConvertFrom-Json -} catch { - Write-Host "⚠️ Failed to fetch commit info: $_" -ForegroundColor Yellow - $commitJson = $null -} -$commitTitle = if ($commitJson) { ($commitJson.message -split "`n")[0] } else { "Unknown" } -$commitTitle = $commitTitle -replace '&','&' -replace '<','<' -replace '>','>' -$commitSha7 = if ($commitJson) { $commitJson.sha.Substring(0, 7) } else { "unknown" } -$commitFull = if ($commitJson) { $commitJson.sha } else { "" } -$commitUrl = if ($commitJson) { "https://github.com/dotnet/maui/commit/$commitFull" } else { "#" } - -try { - $prAuthor = gh api "repos/dotnet/maui/pulls/$PRNumber" --jq '.user.login' 2>$null -} catch { $prAuthor = $null } - -$timestamp = (Get-Date).ToUniversalTime().ToString("yyyy-MM-dd HH:mm UTC") - -# ============================================================================ -# BUILD NEW SESSION BLOCK -# ============================================================================ - -$sessionMarkerStart = "" -$sessionMarkerEnd = "" - -$newSessionBlock = @" -$sessionMarkerStart -
-🚦 Gate Session — $commitSha7 · $commitTitle · $timestamp - ---- - -$gateContent - ---- - -
-$sessionMarkerEnd -"@ - -# ============================================================================ -# MERGE WITH EXISTING SESSIONS -# ============================================================================ - -function Merge-Sessions { - param( - [string]$ExistingBody, - [string]$NewSession, - [string]$CommitSha7 - ) - - $sessionPattern = '(?s).*?' - $existingSessions = [regex]::Matches($ExistingBody, $sessionPattern) - - $sessions = [ordered]@{} - foreach ($match in $existingSessions) { - $sha = $match.Groups[1].Value - $sessions[$sha] = $match.Value - } - - $sessions[$CommitSha7] = $NewSession - - $orderedKeys = @($CommitSha7) + @($sessions.Keys | Where-Object { $_ -ne $CommitSha7 }) - - $allSessions = @() - $isFirst = $true - foreach ($sha in $orderedKeys) { - $block = $sessions[$sha] - if ($isFirst) { - $block = $block -replace '', '
' - $isFirst = $false - } else { - $block = $block -replace '', '
' - } - $allSessions += $block - } - - return ($allSessions -join "`n`n---`n`n") -} - -# ============================================================================ -# FIND EXISTING COMMENT & BUILD FINAL BODY -# ============================================================================ - -Write-Host "Checking for existing gate comment..." -ForegroundColor Yellow -$existingCommentId = $null -$existingBody = $null - -$existingRaw = gh api "repos/dotnet/maui/issues/$PRNumber/comments" --paginate 2>$null -$existingObj = $null -if ($existingRaw) { - try { - $allComments = $existingRaw | ConvertFrom-Json - $existingObj = @($allComments | Where-Object { $_.body -and $_.body.Contains($MARKER) }) | Select-Object -Last 1 - } catch { - Write-Host "⚠️ Could not parse comments: $_" -ForegroundColor Yellow - } -} - -if ($existingObj -and $existingObj.id) { - $existingCommentId = $existingObj.id - $existingBody = $existingObj.body - Write-Host "✓ Found existing comment (ID: $existingCommentId)" -ForegroundColor Green -} - -$authorPing = "" -if ($prAuthor) { - $authorPing = "> 👋 @$prAuthor — new gate results are available. Please review the latest session below." -} - -if ($existingBody) { - $mergedSessions = Merge-Sessions -ExistingBody $existingBody -NewSession $newSessionBlock -CommitSha7 $commitSha7 - - $commentBody = @" -$MARKER - -## 🚦 Gate — Test Before and After Fix - -$authorPing - -$mergedSessions -"@ -} else { - $commentBody = @" -$MARKER - -## 🚦 Gate — Test Before and After Fix - -$authorPing - -$newSessionBlock -"@ -} - -$commentBody = $commentBody -replace "`n{4,}", "`n`n`n" - -# ============================================================================ -# DRY RUN -# ============================================================================ - -if ($DryRun) { - Write-Host "" - Write-Host "=== GATE COMMENT PREVIEW ===" -ForegroundColor Cyan - Write-Host $commentBody - Write-Host "=== END PREVIEW ===" -ForegroundColor Cyan - exit 0 -} - -# ============================================================================ -# POST OR UPDATE COMMENT -# ============================================================================ - -$tempFile = [System.IO.Path]::GetTempFileName() -try { - @{ body = $commentBody } | ConvertTo-Json -Depth 10 | Set-Content -Path $tempFile -Encoding UTF8 - - if ($existingCommentId) { - Write-Host "Updating gate comment (ID: $existingCommentId)..." -ForegroundColor Yellow - try { - gh api --method PATCH "repos/dotnet/maui/issues/comments/$existingCommentId" --input $tempFile 2>&1 | Out-Null - if ($LASTEXITCODE -ne 0) { throw "PATCH failed" } - Write-Host "✅ Gate comment updated" -ForegroundColor Green - Write-Output "COMMENT_ID=$existingCommentId" - } catch { - Write-Host "⚠️ Could not update comment $existingCommentId : $_" -ForegroundColor Yellow - $botLogin = gh api user --jq .login 2>$null - if ($botLogin) { - $ownRaw = gh api "repos/dotnet/maui/issues/$PRNumber/comments" --paginate 2>$null - $ownCommentId = $null - if ($ownRaw) { - try { - $ownAll = $ownRaw | ConvertFrom-Json - $ownMatch = @($ownAll | Where-Object { $_.body -and $_.body.Contains($MARKER) -and $_.user.login -eq $botLogin }) | Select-Object -Last 1 - if ($ownMatch) { $ownCommentId = $ownMatch.id } - } catch { } - } - if ($ownCommentId -and $ownCommentId -ne "null") { - Write-Host " Retrying with own comment (ID: $ownCommentId)..." -ForegroundColor Yellow - gh api --method PATCH "repos/dotnet/maui/issues/comments/$ownCommentId" --input $tempFile 2>&1 | Out-Null - if ($LASTEXITCODE -eq 0) { - Write-Host "✅ Gate comment updated (own comment)" -ForegroundColor Green - Write-Output "COMMENT_ID=$ownCommentId" - return - } - } - } - Write-Host " Creating new comment as fallback..." -ForegroundColor Yellow - $newJson = gh api --method POST "repos/dotnet/maui/issues/$PRNumber/comments" --input $tempFile - $newId = ($newJson | ConvertFrom-Json).id - Write-Host "✅ Gate comment posted (ID: $newId)" -ForegroundColor Green - Write-Output "COMMENT_ID=$newId" - } - } else { - Write-Host "Creating new gate comment..." -ForegroundColor Yellow - $newJson = gh api --method POST "repos/dotnet/maui/issues/$PRNumber/comments" --input $tempFile - $newId = ($newJson | ConvertFrom-Json).id - Write-Host "✅ Gate comment posted (ID: $newId)" -ForegroundColor Green - Write-Output "COMMENT_ID=$newId" - } -} finally { - Remove-Item $tempFile -ErrorAction SilentlyContinue -} diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index ab07ead11c2f..3f0b145ffa04 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -17,7 +17,7 @@ Standalone skill that evaluates PR code changes for correctness, safety, perform **Do NOT use for:** "what does PR #XXXXX do?", "summarize PR", "describe the changes", or any informational query — just answer those directly without invoking this skill. > **How this differs from other skills:** -> - **`pr-review`** — End-to-end PR workflow (4 phases: pre-flight, gate, try-fix, report). Use when you want the full pipeline including test verification and fix attempts. +> - **`pr-review`** — End-to-end PR workflow (3 phases: pre-flight with code review, try-fix, report; gate runs separately). Use when you want the full pipeline including test verification and fix attempts. Pre-Flight invokes this skill as a sub-agent for independence-first code analysis. > - **`pr-finalize`** — Verifies PR title/description match implementation + light code review. Use before merging. > - **`code-review`** (this skill) — Deep code-only review with MAUI domain rules. Use when you want a thorough code analysis without running tests or modifying the PR. diff --git a/.github/skills/pr-review/SKILL.md b/.github/skills/pr-review/SKILL.md index fa17c664d410..eab358b1d112 100644 --- a/.github/skills/pr-review/SKILL.md +++ b/.github/skills/pr-review/SKILL.md @@ -18,7 +18,7 @@ End-to-end PR review workflow that orchestrates phases to explore independent fi ``` Gate (pre-run) → Already completed by Review-PR.ps1 before this skill runs -Phase 1: Pre-Flight → Gather context, classify files → .github/pr-review/pr-preflight.md +Phase 1: Pre-Flight → Gather context, classify files, code review → .github/pr-review/pr-preflight.md Phase 2: Try-Fix → ⚠️ MANDATORY multi-model exploration → invoke try-fix skill (×4 models) Phase 3: Report → Write review recommendation → .github/pr-review/pr-report.md ``` @@ -26,6 +26,7 @@ Phase 3: Report → Write review recommendation → .g > **Gate and Branch setup** are handled by `Review-PR.ps1` before this skill is invoked. The gate result is passed in the prompt. Do NOT re-run gate verification. **All phases write output to:** `CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/{phase}/content.md` +**Pre-Flight also writes:** `CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/pre-flight/code-review.md` --- @@ -44,7 +45,7 @@ Phase 3: Report → Write review recommendation → .g ### Multi-Model Configuration -Phase 3 uses these 4 AI models (run SEQUENTIALLY — they modify the same files): +Phase 2 uses these 4 AI models (run SEQUENTIALLY — they modify the same files): | Order | Model | |-------|-------| @@ -71,10 +72,20 @@ Phase 3 uses these 4 AI models (run SEQUENTIALLY — they modify the same files) > Read and follow `.github/pr-review/pr-preflight.md` -Gather context from the issue, PR, comments, and classify changed files. +Gather context from the issue, PR, comments, classify changed files, and **perform a deep code review** using the `code-review` skill. + +Pre-Flight now has two parts: +- **Part A (Steps 1–6):** Context gathering — read issue, PR, comments, classify files +- **Part B (Step 7):** Code review — independence-first code analysis using `.github/skills/code-review/SKILL.md` and `.github/skills/code-review/references/review-rules.md` + +**Outputs:** +- `pre-flight/content.md` — Context + code review summary +- `pre-flight/code-review.md` — Full code-review output (findings, blast radius, failure-mode probes, verdict) **Gate:** None — always runs. +**Why code review runs here:** The code-review findings (❌ Errors, ⚠️ Warnings, failure-mode probes, blast radius) become **structured hints for Phase 2 (Try-Fix)**. Instead of each model starting from scratch, they receive concrete code concerns to address, leading to higher-quality fix exploration. + --- ## Phase 2: Try-Fix → Invoke `try-fix` Skill (×4 Models) @@ -87,6 +98,8 @@ Even if the PR's fix looks correct and Gate passed, you MUST still run all 4 mod ### 🚨 CRITICAL: try-fix is Independent of PR's Fix +"Independent" means each model explores a **different fix approach** from the PR's fix — not that models are isolated from code-review context. Code-review findings are provided as advisory background to improve fix quality. + The purpose is NOT to re-test the PR's fix, but to: 1. **Generate independent fix ideas** — What would YOU do to fix this bug? 2. **Test those ideas empirically** — Actually implement and run tests @@ -119,10 +132,27 @@ prompt: | - target_files: - src/{area}/{file1}.cs - src/{area}/{file2}.cs + - hints: | + Code review found the following concerns (advisory — use to inform your approach, not as a checklist): + Errors: + - {❌ Error finding 1 with file:line reference} + # Include warnings ONLY if relevant to the root cause: + # Warnings: + # - {⚠️ Warning — omit if unrelated to root cause} + Failure modes: + - {Failure mode 1}: {What happens in this scenario} + Blast radius: {Summary — e.g., "Runs for ALL toolbar items at startup, not just badged ones"} + Code review verdict: {LGTM / NEEDS_CHANGES / NEEDS_DISCUSSION} (confidence: {high/medium/low}) Generate ONE independent fix idea. Review the PR's fix first to ensure your approach is DIFFERENT. + "Independent" means exploring a different fix approach — the code review context above is background + information to help you make better decisions, not a constraint on your exploration. ``` +**Include code review context in the `hints` field** (try-fix's documented optional input). If Pre-Flight code review found no issues, use `hints: "Code review found no issues (verdict: LGTM)"`. If code review was SKIPPED, omit the `hints` field entirely. + +**Selectivity:** Only include ❌ Error findings and failure-mode probes that are relevant to the bug being fixed. Omit 💡 Suggestions. Include ⚠️ Warnings only if directly related to the root cause. + **Wait for each to complete before starting the next.** **🧹 MANDATORY: Clean up between attempts:** @@ -198,7 +228,7 @@ Deliver the final review recommendation. > 🚨 **DO NOT post any comments.** All output goes to `CustomAgentLogsTmp/PRState/`. -**Gate:** Phases 1-3 must be complete. +**Gate:** Phases 1-2 must be complete. --- @@ -207,18 +237,19 @@ Deliver the final review recommendation. ``` CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/ ├── pre-flight/ -│ └── content.md # Phase 1 output (pr-preflight) +│ ├── content.md # Phase 1 output (context + code review summary) +│ └── code-review.md # Full code-review skill output (findings, blast radius, verdict) ├── gate/ -│ └── content.md # Phase 2 output (pr-gate) +│ └── content.md # Gate output (pr-gate, run separately) ├── try-fix/ -│ ├── content.md # Phase 3 summary +│ ├── content.md # Phase 2 summary │ └── attempt-{N}/ # Per-model attempt │ ├── approach.md # What was tried │ ├── result.txt # Pass / Fail / Blocked │ ├── fix.diff # git diff of changes │ └── analysis.md # Why it worked/failed └── report/ - └── content.md # Phase 4 output (pr-report) + └── content.md # Phase 3 output (pr-report) ``` --- @@ -227,10 +258,10 @@ CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/ | Phase | Instructions | Key Action | If Blocked | |-------|--------------|------------|------------| -| 1. Pre-Flight | `pr-preflight.md` | Read issue + PR context | Skip missing info, continue | -| 2. Gate | `pr-gate.md` | Verify tests via task agent | Document, continue to Try-Fix | -| 3. Try-Fix | `try-fix` skill (×4) | **4-model exploration (MANDATORY)** | Skip failing models, continue | -| 4. Report | `pr-report.md` | Write review recommendation | Never skip | +| Gate (pre-run) | `pr-gate.md` | Verify tests (run by Review-PR.ps1) | Result passed in prompt — if missing, document and continue | +| 1. Pre-Flight | `pr-preflight.md` | Read issue + PR context + **code review** | Skip missing info; if code review fails, set verdict to SKIPPED | +| 2. Try-Fix | `try-fix` skill (×4) | **4-model exploration with code-review hints (MANDATORY)** | Skip failing models, continue | +| 3. Report | `pr-report.md` | Write review recommendation | Never skip | --- diff --git a/.github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 b/.github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 index db77a90210be..5a94bf00fb2a 100644 --- a/.github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 +++ b/.github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 @@ -274,7 +274,11 @@ try { ) # Add RuntimeIdentifier if specified - if ($platformConfig.RuntimeIdentifier) { + # NOTE: For Windows we deliberately do NOT pass `-r` here; RuntimeIdentifierOverride + # is set in the windows-specific block below to ensure the RID propagates to ALL + # referenced projects (e.g. TestUtils.DeviceTests). Plain `-r` is suppressed on + # non-leaf project references and causes PRI/asset file resolution failures. + if ($platformConfig.RuntimeIdentifier -and $Platform -ne "windows") { $buildArgs += "-r", $platformConfig.RuntimeIdentifier } @@ -296,8 +300,27 @@ try { $buildArgs += "/p:AndroidPackageFormat=apk" } "windows" { + # NOTE: WindowsAppSDKSelfContained MUST NOT be passed via command line because it + # propagates to ALL referenced projects (including library dependencies like + # Graphics.csproj) and breaks them with: + # "WindowsAppSDKSelfContained requires a supported Windows architecture" + # Instead, pass _MauiDeviceTestUnpackaged=true. The + # Microsoft.Maui.TestUtils.DeviceTests.Runners.props file (imported from each + # device test csproj) converts that signal into WindowsAppSDKSelfContained=true + # ONLY on the device test project itself. + # + # Also: use RuntimeIdentifierOverride (NOT `-r`/RuntimeIdentifier) so the RID + # propagates to every ProjectReference (e.g. TestUtils.DeviceTests). Plain + # RuntimeIdentifier is auto-suppressed on non-leaf project references, which + # leaves dependency PRI/asset files in the non-RID output folder while the + # test app itself is built at the RID-specific path, producing PRI175 errors. + # + # See eng/devices/windows.cake (buildOnly task, lines 145-188) for the + # canonical CI pattern. + $buildArgs += "/p:RuntimeIdentifierOverride=$($platformConfig.RuntimeIdentifier)" $buildArgs += "/p:WindowsPackageType=None" - $buildArgs += "/p:WindowsAppSDKSelfContained=true" + $buildArgs += "/p:SelfContained=true" + $buildArgs += "/p:_MauiDeviceTestUnpackaged=true" $buildArgs += "/p:UseMonoRuntime=false" } }