diff --git a/.github/agents/learn-from-pr.md b/.github/agents/learn-from-pr.md new file mode 100644 index 000000000000..ce0a506c3af3 --- /dev/null +++ b/.github/agents/learn-from-pr.md @@ -0,0 +1,113 @@ +--- +name: learn-from-pr +description: Analyzes completed PRs for lessons learned, then applies improvements to instruction files, skills, and documentation. +--- + +# Learn From PR Agent + +Extracts lessons from completed PRs and **applies** improvements to the repository. + +## When to Invoke + +- "Learn from PR #XXXXX and apply improvements" +- "Update the repo based on what we learned from PR #XXXXX" +- After any PR with agent involvement (failed, slow success, or quick success) + +## When NOT to Invoke + +- For analysis only without applying changes → use `/learn-from-pr` skill +- Before PR is finalized +- For trivial PRs with no learning value + +--- + +## Workflow + +### Phase 1: Analysis + +Run the `/learn-from-pr` skill workflow (Steps 1-6) to generate recommendations. + +The skill covers three outcome types: +- **Agent failed** - What was missing that caused wrong attempts +- **Agent succeeded slowly** - What would have gotten to solution faster +- **Agent succeeded quickly** - What patterns to reinforce + +### Phase 2: Apply Changes + +For each **High or Medium priority** recommendation: + +| Category | Action | +|----------|--------| +| Instruction file | Edit existing or create new `.github/instructions/*.instructions.md` | +| Skill enhancement | Edit `.github/skills/*/SKILL.md` | +| Architecture doc | Edit `/docs/design/*.md` (detailed) or create quick-reference in `.github/architecture/` | +| General AI guidance | Edit `.github/copilot-instructions.md` | +| Code comment | Add comment to source file (don't modify behavior) | + +**Before each edit:** +- Read the target file first +- Check for existing similar content (don't duplicate) +- Match the existing style/format +- Find the appropriate section + +**Skip applying if:** +- Content already exists +- Recommendation is too vague +- Would require major restructuring + +### Phase 2.5: Verify Changes + +After applying changes: + +1. Run `git diff` to review all edits +2. Verify no syntax errors in modified files (valid markdown) +3. Confirm style matches existing content +4. If issues found, fix or revert before reporting + +### Phase 3: Report + +Present a summary: + +```markdown +## Changes Applied + +| File | Change | +|------|--------| +| [path] | [what was added/modified] | + +## Not Applied + +| Recommendation | Reason | +|----------------|--------| +| [rec] | [why skipped] | +``` + +--- + +## Error Handling + +| Situation | Action | +|-----------|--------| +| PR not found | Ask user to verify PR number | +| No session markdown | Proceed with PR diff analysis only | +| Target file doesn't exist | Create if instruction/architecture doc, skip if code | +| Duplicate content exists | Skip, note in report | +| Unclear where to add | Ask user for guidance | + +## Constraints + +- **Only apply High/Medium priority** - Report Low priority without applying +- **Don't duplicate** - Check existing content first +- **Match style** - Read file before editing +- **Code comments only** - Never modify code behavior +- **No linter implementation** - File issue instead of building analyzers + +--- + +## Difference from Skill + +| Aspect | `/learn-from-pr` Skill | This Agent | +|--------|------------------------|------------| +| Output | Recommendations to discuss | Applied changes | +| Mode | Analysis only | Autonomous | +| Use when | Want to review without applying | CI automation | diff --git a/.github/agents/pr.md b/.github/agents/pr.md index c7d4d8600e12..89399216fe19 100644 --- a/.github/agents/pr.md +++ b/.github/agents/pr.md @@ -39,6 +39,23 @@ After Gate passes, read `.github/agents/pr/post-gate.md` for **Phases 4-5**. └─────────────────────────────────────────┘ └─────────────────────────────────────────────┘ ``` +--- + +## Phase Completion Protocol (CRITICAL) + +**Before changing ANY phase status to ✅ COMPLETE:** + +1. **Read the state file section** for the phase you're completing +2. **Find ALL ⏳ PENDING and [PENDING] fields** in that section +3. **Fill in every field** with actual content +4. **Verify no pending markers remain** in your section +5. **Commit the state file** with complete content +6. **Then change status** to ✅ COMPLETE + +**Rule:** Status ✅ means "documentation complete", not "I finished thinking about it" + +--- + ### 🚨 CRITICAL: Phase 4 Always Uses `try-fix` Skill **Even when a PR already has a fix**, Phase 4 requires running the `try-fix` skill to: @@ -326,6 +343,14 @@ The test result will be updated to `✅ PASS (Gate)` after Gate passes. 3. Add edge cases and any disagreements (if PR exists) 4. Change 🧪 Tests status to `▶️ IN PROGRESS` +**Before marking ✅ COMPLETE, verify state file contains:** +- [ ] Issue summary filled (not [PENDING]) +- [ ] Platform checkboxes marked +- [ ] Files Changed table populated (if PR exists) +- [ ] PR Discussion Summary documented (if PR exists) +- [ ] All [PENDING] placeholders replaced +- [ ] State file committed + --- ## 🧪 TESTS: Create/Verify Reproduction Tests (Phase 2) @@ -385,6 +410,12 @@ The script auto-detects mode based on git diff. If only test files changed, it v 4. Change 🧪 Tests status to `✅ COMPLETE` 5. Change 🚦 Gate status to `▶️ IN PROGRESS` +**Before marking ✅ COMPLETE, verify state file contains:** +- [ ] Test file paths documented +- [ ] "Tests verified to FAIL" note added +- [ ] Test category identified +- [ ] State file committed + --- ## 🚦 GATE: Verify Tests Catch the Issue (Phase 3) @@ -432,6 +463,12 @@ pwsh .github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1 2. Change 🚦 Gate status to `✅ PASSED` 3. Proceed to Phase 4 +**Before marking ✅ PASSED, verify state file contains:** +- [ ] Result shows PASSED ✅ or FAILED ❌ +- [ ] Test behavior documented +- [ ] Platform tested noted +- [ ] State file committed + --- ## ⛔ STOP HERE diff --git a/.github/agents/pr/post-gate.md b/.github/agents/pr/post-gate.md index c7ccb5033432..4dbfa136e6f7 100644 --- a/.github/agents/pr/post-gate.md +++ b/.github/agents/pr/post-gate.md @@ -15,6 +15,21 @@ If Gate is not passed, go back to `.github/agents/pr.md` and complete phases 1-3 --- +## Phase Completion Protocol (CRITICAL) + +**Before changing ANY phase status to ✅ COMPLETE:** + +1. **Read the state file section** for the phase you're completing +2. **Find ALL ⏳ PENDING and [PENDING] fields** in that section +3. **Fill in every field** with actual content +4. **Verify no pending markers remain** in your section +5. **Commit the state file** with complete content +6. **Then change status** to ✅ COMPLETE + +**Rule:** Status ✅ means "documentation complete", not "I finished thinking about it" + +--- + ## 🔧 FIX: Explore and Select Fix (Phase 4) > **SCOPE**: Explore independent fix alternatives using `try-fix` skill, compare with PR's fix, select the best approach. @@ -37,6 +52,15 @@ The purpose of Phase 4 is NOT to re-test the PR's fix, but to: Invoke the `try-fix` skill repeatedly. The skill handles one fix attempt per invocation. +**IMPORTANT:** Always pass the `state_file` parameter so try-fix can record its results: +``` +state_file: .github/agent-pr-session/pr-XXXXX.md +``` + +try-fix will automatically append rows to the Fix Candidates table and set the "Exhausted" field. You remain responsible for: +- Setting "Selected Fix" field with reasoning +- Updating phase status to ✅ COMPLETE + ``` ┌─────────────────────────────────────────────────────────────┐ │ Agent orchestration loop │ @@ -44,18 +68,21 @@ Invoke the `try-fix` skill repeatedly. The skill handles one fix attempt per inv │ │ │ attempts = 0 │ │ max_attempts = 5 │ +│ state_file = ".github/agent-pr-session/pr-XXXXX.md" │ │ │ │ while (attempts < max_attempts): │ -│ result = invoke try-fix skill │ +│ result = invoke try-fix skill (with state_file) │ │ attempts++ │ │ │ │ if result.exhausted: │ │ break # try-fix has no more ideas │ │ │ │ # result.passed indicates if this attempt worked │ +│ # try-fix already recorded to state file │ │ # Continue loop to explore more alternatives │ │ │ │ # After loop: compare all try-fix results vs PR's fix │ +│ # Update "Exhausted" and "Selected Fix" fields │ │ │ └─────────────────────────────────────────────────────────────┘ ``` @@ -73,8 +100,9 @@ Each `try-fix` invocation: 3. Proposes ONE new independent fix idea 4. Implements and tests it 5. Records result (with failure analysis if it failed) -6. Reverts all changes (restores PR's fix) -7. Returns `{passed: bool, exhausted: bool}` +6. **Updates state file** (appends row to Fix Candidates table if state_file provided) +7. Reverts all changes (restores PR's fix) +8. Returns `{passed: bool, exhausted: bool}` See `.github/skills/try-fix/SKILL.md` for full details. @@ -134,6 +162,15 @@ Update the state file: 4. Change 🔧 Fix status to `✅ COMPLETE` 5. Change 📋 Report status to `▶️ IN PROGRESS` +**Before marking ✅ COMPLETE, verify state file contains:** +- [ ] Root Cause Analysis filled in (if applicable) +- [ ] Fix Candidates table has numbered rows for each try-fix attempt +- [ ] Each row has: approach, test result, files changed, notes +- [ ] "Exhausted" field set (Yes/No) +- [ ] "Selected Fix" populated with reasoning +- [ ] No ⏳ PENDING markers remain in Fix section +- [ ] State file committed + --- ## 📋 REPORT: Final Report (Phase 5) @@ -233,6 +270,13 @@ Update all phase statuses to complete. 2. Update all phases to `✅ COMPLETE` or `✅ PASSED` 3. Present final result to user +**Before marking ✅ COMPLETE, verify state file contains:** +- [ ] Final recommendation (APPROVE/REQUEST_CHANGES/COMMENT) +- [ ] Summary of findings +- [ ] Key technical insights documented +- [ ] Overall status changed to final recommendation +- [ ] State file committed + --- ## Common Mistakes in Post-Gate Phases diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 43a1e9aa8fbf..604d89989798 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -6,6 +6,10 @@ description: "Guidance for GitHub Copilot when working on the .NET MAUI reposito This document provides specific guidance for GitHub Copilot when working on the .NET MAUI repository. It serves as context for understanding the project structure, development workflow, and best practices. +## Code Review Instructions + +When performing a code review on PRs that change functional code, run the pr-finalize skill to verify that the PR title and description accurately match the actual implementation. This ensures proper documentation and helps maintain high-quality commit messages. + ## Repository Overview **.NET MAUI** is a cross-platform framework for creating mobile and desktop applications with C# and XAML. This repository contains the core framework code that enables development for Android, iOS, iPadOS, macOS, and Windows from a single shared codebase. @@ -177,9 +181,18 @@ Always put that at the top, without the block quotes. Without it, users will NOT -## Custom Agents +## Custom Agents and Skills + +The repository includes specialized custom agents and reusable skills for specific tasks. + +### Skills vs Agents -The repository includes specialized custom agents for specific tasks. These agents are available to GitHub Copilot and can be invoked for their respective specializations: +| Aspect | Skills | Agents | +|--------|--------|--------| +| **Invoke** | `/skill-name` or direct request | Delegate to agent | +| **Output** | Analysis, recommendations | Actions, changes applied | +| **Interaction** | Interactive discussion | Autonomous workflow | +| **Example** | `/learn-from-pr` → recommendations | learn-from-pr agent → applies changes | ### Available Custom Agents @@ -200,15 +213,65 @@ The repository includes specialized custom agents for specific tasks. These agen - **Trigger phrases**: "test this PR", "validate PR #XXXXX in Sandbox", "reproduce issue #XXXXX", "try out in Sandbox" - **Do NOT use for**: Code review (use pr agent), writing automated tests (use uitest-coding-agent) +4. **learn-from-pr** - Extracts lessons from PRs and applies improvements to the repository + - **Use when**: After complex PR, want to improve instruction files/skills based on lessons learned + - **Capabilities**: Analyzes PR, identifies failure modes, applies improvements to instruction files, skills, code comments + - **Trigger phrases**: "learn from PR #XXXXX and apply improvements", "improve repo based on what we learned", "update skills based on PR" + - **Output**: Applied changes to instruction files, skills, architecture docs, code comments + - **Do NOT use for**: Analysis only without applying changes → Use `/learn-from-pr` skill instead + ### Reusable Skills Skills are modular capabilities that can be invoked directly or used by agents. Located in `.github/skills/`: +#### User-Facing Skills + 1. **issue-triage** (`.github/skills/issue-triage/SKILL.md`) - **Purpose**: Query and triage open issues that need milestones, labels, or investigation - - **Trigger phrases**: "find issues to triage", "show me old Android issues", "what issues need attention", "triage Android issues" + - **Trigger phrases**: "find issues to triage", "show me old Android issues", "what issues need attention" - **Scripts**: `init-triage-session.ps1`, `query-issues.ps1`, `record-triage.ps1` - - **Used by**: Any agent or direct invocation + +2. **find-reviewable-pr** (`.github/skills/find-reviewable-pr/SKILL.md`) + - **Purpose**: Finds open PRs in dotnet/maui and dotnet/docs-maui that need review + - **Trigger phrases**: "find PRs to review", "show milestoned PRs", "find partner PRs" + - **Scripts**: `query-reviewable-prs.ps1` + - **Categories**: P/0, milestoned, partner, community, recent, docs-maui + +3. **pr-finalize** (`.github/skills/pr-finalize/SKILL.md`) + - **Purpose**: Verifies PR title and description match actual implementation. Works on any PR. Optionally updates agent session markdown if present. + - **Trigger phrases**: "finalize PR #XXXXX", "check PR description for #XXXXX", "review commit message" + - **Used by**: Before merging any PR, when description may be stale + - **Note**: Does NOT require agent involvement or session markdown - works on any PR + +4. **learn-from-pr** (`.github/skills/learn-from-pr/SKILL.md`) + - **Purpose**: Analyzes completed PR to identify repository improvements (analysis only, no changes applied) + - **Trigger phrases**: "what can we learn from PR #XXXXX?", "how can we improve agents based on PR #XXXXX?" + - **Used by**: After complex PRs, when agent struggled to find solution + - **Output**: Prioritized recommendations for instruction files, skills, code comments + - **Note**: For applying changes automatically, use the learn-from-pr agent instead + +5. **write-tests** (`.github/skills/write-tests/SKILL.md`) + - **Purpose**: Creates UI tests for GitHub issues and verifies they reproduce the bug + - **Trigger phrases**: "write tests for #XXXXX", "create test for issue", "add UI test coverage" + - **Output**: Test files that fail without fix, pass with fix + +6. **verify-tests-fail-without-fix** (`.github/skills/verify-tests-fail-without-fix/SKILL.md`) + - **Purpose**: Verifies UI tests catch the bug before fix and pass with fix + - **Two modes**: Verify failure only (test creation) or full verification (test + fix) + - **Used by**: After creating tests, before considering PR complete + +7. **pr-build-status** (`.github/skills/pr-build-status/SKILL.md`) + - **Purpose**: Retrieves Azure DevOps build information for PRs (build IDs, stage status, failed jobs) + - **Trigger phrases**: "check build for PR #XXXXX", "why did PR build fail", "get build status" + - **Used by**: When investigating CI failures + +#### Internal Skills (Used by Agents) + +8. **try-fix** (`.github/skills/try-fix/SKILL.md`) + - **Purpose**: Proposes ONE independent fix approach, applies it, tests, records result with failure analysis, then reverts + - **Used by**: pr agent Phase 3 (Fix phase) - rarely invoked directly by users + - **Behavior**: Reads prior attempts to learn from failures. Max 5 attempts per session. + - **Output**: Updates session markdown with attempt results and failure analysis ### Using Custom Agents diff --git a/.github/scripts/EstablishBrokenBaseline.ps1 b/.github/scripts/EstablishBrokenBaseline.ps1 new file mode 100644 index 000000000000..4014eec522be --- /dev/null +++ b/.github/scripts/EstablishBrokenBaseline.ps1 @@ -0,0 +1,482 @@ +#!/usr/bin/env pwsh +<# +.SYNOPSIS + Establishes a "broken" baseline by reverting fix files to their merge-base state. + +.DESCRIPTION + This script provides reusable baseline logic for test verification workflows. + It handles: + - Finding the merge-base commit (supports fork workflows, PR metadata, etc.) + - Detecting fix files (non-test files that changed since merge-base) + - Reverting fix files to create a "broken" state for testing + - Restoring files back to their current state + + Used by verify-tests-fail.ps1 and try-fix skill. + +.PARAMETER BaseBranch + Optional explicit base branch name. If not provided, auto-detects from PR metadata + or finds the closest merge-base among common branch patterns. + +.PARAMETER FixFiles + Optional array of explicit fix files. If not provided, auto-detects from git diff + by excluding test directories. + +.PARAMETER DryRun + Report what would be done without making any changes. + +.PARAMETER Restore + Restore previously reverted files from HEAD. + +.EXAMPLE + # Establish baseline (revert fix files) + $baseline = ./EstablishBrokenBaseline.ps1 + # Run tests... + ./EstablishBrokenBaseline.ps1 -Restore + +.EXAMPLE + # Dry run - see what would be reverted + ./EstablishBrokenBaseline.ps1 -DryRun + +.EXAMPLE + # Explicit base branch + $baseline = ./EstablishBrokenBaseline.ps1 -BaseBranch main +#> + +param( + [Parameter(Mandatory = $false)] + [string]$BaseBranch, + + [Parameter(Mandatory = $false)] + [string[]]$FixFiles, + + [Parameter(Mandatory = $false)] + [switch]$DryRun, + + [Parameter(Mandatory = $false)] + [switch]$Restore +) + +$ErrorActionPreference = "Stop" +$RepoRoot = git rev-parse --show-toplevel + +# ============================================================ +# Test path patterns to exclude when auto-detecting fix files +# ============================================================ +$script:TestPathPatterns = @( + "*/tests/*", + "*/test/*", + "*.Tests/*", + "*.UnitTests/*", + "*TestCases*", + "*snapshots*", + "*.png", + "*.jpg", + ".github/*", + "*.md", + "pr-*-review.md" +) + +# ============================================================ +# Function to check if a file should be excluded from fix files +# ============================================================ +function Test-IsTestFile { + param([string]$FilePath) + + foreach ($pattern in $script:TestPathPatterns) { + if ($FilePath -like $pattern) { + return $true + } + } + return $false +} + +# ============================================================ +# Find the merge-base commit (where current branch diverged from base) +# This is more robust than tracking branch names/refs +# For fork workflows: fetches directly from the PR's target repo URL +# so it works even if the fork's main branch is out of sync +# ============================================================ +function Find-MergeBase { + param([string]$ExplicitBaseBranch) + + # 1. If explicit base branch provided, use it directly + if ($ExplicitBaseBranch) { + # Try with origin/ prefix first, then without + foreach ($ref in @("origin/$ExplicitBaseBranch", $ExplicitBaseBranch)) { + $mergeBase = git merge-base HEAD $ref 2>$null + if ($mergeBase) { + return @{ MergeBase = $mergeBase; BaseBranch = $ExplicitBaseBranch; Source = "explicit" } + } + } + } + + # 2. Try to get PR metadata including the TARGET repository + # This is critical for fork workflows where origin points to the fork, + # not the upstream repo. We fetch directly from the target repo URL. + # The PR URL contains the target repo: https://github.com/OWNER/REPO/pull/123 + $prJson = gh pr view --json baseRefName,url 2>$null + if ($prJson) { + $prInfo = $prJson | ConvertFrom-Json + $prBaseBranch = $prInfo.baseRefName + $prUrl = $prInfo.url + + # Parse owner/repo from PR URL: https://github.com/OWNER/REPO/pull/123 + $targetOwner = $null + $targetRepo = $null + if ($prUrl -match "github\.com/([^/]+)/([^/]+)/pull/") { + $targetOwner = $matches[1] + $targetRepo = $matches[2] + } + + if ($prBaseBranch -and $targetOwner -and $targetRepo) { + # Construct the target repo URL and fetch directly from it + # This works even if the developer hasn't set up an 'upstream' remote + # and even if their fork's main is completely out of sync + $targetUrl = "https://github.com/$targetOwner/$targetRepo.git" + Write-Host "PR targets $targetOwner/$targetRepo - fetching $prBaseBranch from upstream..." -ForegroundColor Cyan + git fetch $targetUrl $prBaseBranch 2>$null + + if ($LASTEXITCODE -eq 0) { + # FETCH_HEAD now points to the target repo's base branch + $mergeBase = git merge-base HEAD FETCH_HEAD 2>$null + if ($mergeBase) { + return @{ MergeBase = $mergeBase; BaseBranch = $prBaseBranch; Source = "pr-target-repo"; TargetRepo = "$targetOwner/$targetRepo" } + } + } + } + + # Fallback: try fetching from origin (works if origin IS the target repo) + if ($prBaseBranch) { + git fetch origin $prBaseBranch 2>$null + foreach ($ref in @("origin/$prBaseBranch", $prBaseBranch)) { + $mergeBase = git merge-base HEAD $ref 2>$null + if ($mergeBase) { + return @{ MergeBase = $mergeBase; BaseBranch = $prBaseBranch; Source = "pr-metadata" } + } + } + } + } + + # 3. Fallback: Find closest merge-base among common base branch patterns + # The "correct" base is the one with fewest commits between merge-base and HEAD + Write-Host "No PR detected, scanning remote branches for closest base..." -ForegroundColor Cyan + + # Fetch all remote refs to ensure we have latest + git fetch origin 2>$null + + # Get remote branches matching common base branch patterns + $remoteBranches = git branch -r --format='%(refname:short)' 2>$null | Where-Object { + $_ -match '^origin/(main|master|net\d+\.\d+|release/.*)$' + } + + $bestMatch = $null + $shortestDistance = [int]::MaxValue + + foreach ($branch in $remoteBranches) { + $mergeBase = git merge-base HEAD $branch 2>$null + if ($mergeBase) { + $distance = [int](git rev-list --count "$mergeBase..HEAD" 2>$null) + if ($distance -lt $shortestDistance) { + $shortestDistance = $distance + $branchName = $branch -replace '^origin/', '' + $bestMatch = @{ MergeBase = $mergeBase; BaseBranch = $branchName; Source = "closest-merge-base"; Distance = $distance } + } + } + } + + return $bestMatch +} + +# ============================================================ +# Get detected fix files from git diff +# ============================================================ +function Get-FixFiles { + param( + [string]$MergeBase, + [string[]]$ExplicitFixFiles + ) + + # Override with explicitly provided fix files + if ($ExplicitFixFiles -and $ExplicitFixFiles.Count -gt 0) { + return $ExplicitFixFiles + } + + # Auto-detect from git diff + $DetectedFixFiles = @() + $changedFiles = git diff $MergeBase HEAD --name-only 2>$null + + if ($changedFiles) { + foreach ($file in $changedFiles) { + if (-not (Test-IsTestFile $file)) { + $DetectedFixFiles += $file + } + } + } + + return $DetectedFixFiles +} + +# ============================================================ +# Categorize fix files into revertable vs new +# ============================================================ +function Get-FileCategories { + param( + [string]$MergeBase, + [string[]]$FixFiles + ) + + $RevertableFiles = @() + $NewFiles = @() + + foreach ($file in $FixFiles) { + # Check if file exists at merge-base commit + $existsInBase = git ls-tree -r $MergeBase --name-only -- $file 2>$null + + if ($existsInBase) { + $RevertableFiles += $file + } else { + $NewFiles += $file + } + } + + return @{ + RevertableFiles = $RevertableFiles + NewFiles = $NewFiles + } +} + +# ============================================================ +# Revert files to merge-base state +# ============================================================ +function Invoke-RevertFiles { + param( + [string]$MergeBase, + [string[]]$Files + ) + + foreach ($file in $Files) { + git checkout $MergeBase -- $file 2>&1 | Out-Null + if ($LASTEXITCODE -ne 0) { + throw "Failed to revert $file from $MergeBase" + } + } +} + +# ============================================================ +# Restore files from HEAD +# ============================================================ +function Invoke-RestoreFiles { + param([string[]]$Files) + + foreach ($file in $Files) { + git checkout HEAD -- $file 2>&1 | Out-Null + if ($LASTEXITCODE -ne 0) { + throw "Failed to restore $file from HEAD" + } + } +} + +# ============================================================ +# State file for tracking reverted files (for -Restore) +# ============================================================ +$StateFile = Join-Path $RepoRoot ".github/.baseline-state.json" + +function Save-BaselineState { + param([hashtable]$State) + + $stateDir = Split-Path $StateFile -Parent + if (-not (Test-Path $stateDir)) { + New-Item -ItemType Directory -Force -Path $stateDir | Out-Null + } + + $State | ConvertTo-Json -Depth 10 | Set-Content $StateFile +} + +function Get-BaselineState { + if (Test-Path $StateFile) { + return Get-Content $StateFile -Raw | ConvertFrom-Json + } + return $null +} + +function Remove-BaselineState { + if (Test-Path $StateFile) { + Remove-Item $StateFile -Force + } +} + +# ============================================================ +# Main execution (only when run directly, not when dot-sourced) +# ============================================================ + +# Check if script is being dot-sourced (imported) vs run directly +# When dot-sourced, $MyInvocation.InvocationName is "." or "&" +$script:IsBeingDotSourced = $MyInvocation.InvocationName -eq '.' -or $MyInvocation.InvocationName -eq '&' + +if ($script:IsBeingDotSourced) { + # Script is being imported - just export the functions + return +} + +# Handle -Restore mode +if ($Restore) { + $state = Get-BaselineState + if (-not $state) { + Write-Host "No baseline state found. Nothing to restore." -ForegroundColor Yellow + return @{ Restored = $false; Message = "No baseline state found" } + } + + Write-Host "Restoring $($state.RevertedFiles.Count) file(s) from HEAD..." -ForegroundColor Cyan + + foreach ($file in $state.RevertedFiles) { + Write-Host " Restoring: $file" -ForegroundColor Gray + git checkout HEAD -- $file 2>&1 | Out-Null + if ($LASTEXITCODE -ne 0) { + Write-Host " WARNING: Failed to restore $file" -ForegroundColor Yellow + } + } + + Remove-BaselineState + Write-Host "Baseline restored." -ForegroundColor Green + + return @{ + Restored = $true + RestoredFiles = $state.RevertedFiles + } +} + +# Find merge-base +Write-Host "Detecting base branch and merge point..." -ForegroundColor Cyan + +$baseInfo = Find-MergeBase -ExplicitBaseBranch $BaseBranch + +if (-not $baseInfo) { + Write-Host "ERROR: Could not find merge base" -ForegroundColor Red + Write-Host " Tried: PR metadata, common base branches (main, net*.0, release/*)" -ForegroundColor Yellow + Write-Host " Specify -BaseBranch explicitly to fix." -ForegroundColor Yellow + return @{ Success = $false; Error = "Could not find merge base" } +} + +$MergeBase = $baseInfo.MergeBase +$BaseBranchName = $baseInfo.BaseBranch + +if ($baseInfo.TargetRepo) { + Write-Host "PR target: $($baseInfo.TargetRepo) ($BaseBranchName branch)" -ForegroundColor Green +} else { + Write-Host "Base branch: $BaseBranchName (via $($baseInfo.Source))" -ForegroundColor Green +} +Write-Host "Merge base: $($MergeBase.Substring(0, 8))" -ForegroundColor Green +if ($baseInfo.Distance) { + Write-Host " ($($baseInfo.Distance) commits ahead of $BaseBranchName)" -ForegroundColor Gray +} + +# Get fix files +$detectedFixFiles = Get-FixFiles -MergeBase $MergeBase -ExplicitFixFiles $FixFiles + +if ($detectedFixFiles.Count -eq 0) { + Write-Host "No fix files detected." -ForegroundColor Yellow + return @{ + Success = $true + MergeBase = $MergeBase + BaseBranch = $BaseBranchName + RevertedFiles = @() + NewFiles = @() + NoFixFiles = $true + } +} + +Write-Host "Fix files ($($detectedFixFiles.Count)):" -ForegroundColor Cyan +foreach ($file in $detectedFixFiles) { + Write-Host " - $file" -ForegroundColor White +} + +# Categorize files +$categories = Get-FileCategories -MergeBase $MergeBase -FixFiles $detectedFixFiles + +Write-Host "" +Write-Host "File categories:" -ForegroundColor Cyan +foreach ($file in $categories.RevertableFiles) { + Write-Host " [revert] $file" -ForegroundColor White +} +foreach ($file in $categories.NewFiles) { + Write-Host " [new] $file (skipping)" -ForegroundColor Gray +} + +if ($categories.RevertableFiles.Count -eq 0) { + Write-Host "" + Write-Host "No revertable files found. All fix files are new." -ForegroundColor Yellow + return @{ + Success = $true + MergeBase = $MergeBase + BaseBranch = $BaseBranchName + RevertedFiles = @() + NewFiles = $categories.NewFiles + NoRevertableFiles = $true + } +} + +# Check for uncommitted changes on revertable files +$uncommittedFiles = @() +foreach ($file in $categories.RevertableFiles) { + $status = git status --porcelain -- $file 2>$null + if ($status) { + $uncommittedFiles += $file + } +} + +if ($uncommittedFiles.Count -gt 0) { + Write-Host "" + Write-Host "ERROR: Uncommitted changes in fix files:" -ForegroundColor Red + foreach ($file in $uncommittedFiles) { + Write-Host " - $file" -ForegroundColor Yellow + } + Write-Host "Commit changes before running this script." -ForegroundColor Yellow + return @{ Success = $false; Error = "Uncommitted changes"; UncommittedFiles = $uncommittedFiles } +} + +# DryRun mode +if ($DryRun) { + Write-Host "" + Write-Host "[DRY RUN] Would revert $($categories.RevertableFiles.Count) file(s) to merge-base" -ForegroundColor Cyan + return @{ + Success = $true + DryRun = $true + MergeBase = $MergeBase + BaseBranch = $BaseBranchName + WouldRevert = $categories.RevertableFiles + NewFiles = $categories.NewFiles + } +} + +# Revert files +Write-Host "" +Write-Host "Reverting $($categories.RevertableFiles.Count) file(s) to merge-base ($($MergeBase.Substring(0, 8)))..." -ForegroundColor Cyan + +foreach ($file in $categories.RevertableFiles) { + Write-Host " Reverting: $file" -ForegroundColor Gray + git checkout $MergeBase -- $file 2>&1 | Out-Null + if ($LASTEXITCODE -ne 0) { + Write-Host " ERROR: Failed to revert $file" -ForegroundColor Red + return @{ Success = $false; Error = "Failed to revert $file" } + } +} + +# Save state for -Restore +Save-BaselineState @{ + MergeBase = $MergeBase + BaseBranch = $BaseBranchName + RevertedFiles = $categories.RevertableFiles + NewFiles = $categories.NewFiles + Timestamp = (Get-Date -Format "o") +} + +Write-Host "Baseline established. $($categories.RevertableFiles.Count) file(s) reverted." -ForegroundColor Green +Write-Host "Run with -Restore to restore files." -ForegroundColor Cyan + +return @{ + Success = $true + MergeBase = $MergeBase + BaseBranch = $BaseBranchName + RevertedFiles = $categories.RevertableFiles + NewFiles = $categories.NewFiles +} diff --git a/.github/scripts/tests/Test-EstablishBrokenBaseline.ps1 b/.github/scripts/tests/Test-EstablishBrokenBaseline.ps1 new file mode 100644 index 000000000000..7d32a64feebb --- /dev/null +++ b/.github/scripts/tests/Test-EstablishBrokenBaseline.ps1 @@ -0,0 +1,392 @@ +#!/usr/bin/env pwsh +<# +.SYNOPSIS + Tests for EstablishBrokenBaseline.ps1 + +.DESCRIPTION + Validates that the EstablishBrokenBaseline.ps1 script works correctly. + Run these tests to verify the baseline logic after making changes. + +.EXAMPLE + ./Test-EstablishBrokenBaseline.ps1 +#> + +param( + [switch]$Verbose +) + +$ErrorActionPreference = "Stop" +$RepoRoot = git rev-parse --show-toplevel +$ScriptPath = Join-Path $RepoRoot ".github/scripts/EstablishBrokenBaseline.ps1" +$StateFile = Join-Path $RepoRoot ".github/.baseline-state.json" + +# Test tracking +$script:TestsPassed = 0 +$script:TestsFailed = 0 +$script:TestsSkipped = 0 + +function Write-TestResult { + param( + [string]$TestName, + [bool]$Passed, + [string]$Message = "" + ) + + if ($Passed) { + Write-Host " [PASS] $TestName" -ForegroundColor Green + $script:TestsPassed++ + } else { + Write-Host " [FAIL] $TestName" -ForegroundColor Red + if ($Message) { + Write-Host " $Message" -ForegroundColor Yellow + } + $script:TestsFailed++ + } +} + +function Write-TestSkipped { + param([string]$TestName, [string]$Reason) + Write-Host " [SKIP] $TestName - $Reason" -ForegroundColor Yellow + $script:TestsSkipped++ +} + +function Test-Section { + param([string]$Name) + Write-Host "" + Write-Host "=== $Name ===" -ForegroundColor Cyan +} + +# ============================================================ +# Cleanup function +# ============================================================ +function Invoke-Cleanup { + # Restore any changes and clean up state file + if (Test-Path $StateFile) { + Remove-Item $StateFile -Force -ErrorAction SilentlyContinue + } + git checkout -- . 2>$null +} + +# ============================================================ +# Test: Script exists +# ============================================================ +Test-Section "Script Existence" + +Write-TestResult "EstablishBrokenBaseline.ps1 exists" (Test-Path $ScriptPath) + +# ============================================================ +# Test: Dot-source import works +# ============================================================ +Test-Section "Dot-Source Import" + +try { + . $ScriptPath + Write-TestResult "Script can be dot-sourced without errors" $true + + # Verify functions are available + $functionsAvailable = (Get-Command -Name Find-MergeBase -ErrorAction SilentlyContinue) -and + (Get-Command -Name Test-IsTestFile -ErrorAction SilentlyContinue) -and + (Get-Command -Name Get-FixFiles -ErrorAction SilentlyContinue) -and + (Get-Command -Name Get-FileCategories -ErrorAction SilentlyContinue) + Write-TestResult "Required functions are exported" $functionsAvailable +} catch { + Write-TestResult "Script can be dot-sourced without errors" $false $_.Exception.Message +} + +# ============================================================ +# Test: Test-IsTestFile function +# ============================================================ +Test-Section "Test-IsTestFile Function" + +$testCases = @( + # Test files (should return $true) + @{ Path = "src/Controls/tests/TestCases.HostApp/Issues/Issue20772.cs"; Expected = $true; Desc = "TestCases.HostApp file" }, + @{ Path = "src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue20772.cs"; Expected = $true; Desc = "TestCases.Shared.Tests file" }, + @{ Path = "src/Core/tests/UnitTests/SomeTest.cs"; Expected = $true; Desc = "tests directory file" }, + @{ Path = ".github/scripts/SomeScript.ps1"; Expected = $true; Desc = ".github directory file" }, + @{ Path = "README.md"; Expected = $true; Desc = "Markdown file" }, + @{ Path = "docs/screenshot.png"; Expected = $true; Desc = "PNG image" }, + @{ Path = "snapshots/baseline.png"; Expected = $true; Desc = "Snapshots directory" }, + + # Non-test files (should return $false) + @{ Path = "src/Controls/src/Core/Button.cs"; Expected = $false; Desc = "Core source file" }, + @{ Path = "src/Controls/src/Core/Platform/Android/InnerGestureListener.cs"; Expected = $false; Desc = "Platform source file" }, + @{ Path = "src/Core/src/Handlers/ButtonHandler.cs"; Expected = $false; Desc = "Handler file" }, + @{ Path = "src/Essentials/src/Accelerometer/Accelerometer.cs"; Expected = $false; Desc = "Essentials file" } +) + +foreach ($tc in $testCases) { + $result = Test-IsTestFile $tc.Path + Write-TestResult "$($tc.Desc): $($tc.Path)" ($result -eq $tc.Expected) "Expected: $($tc.Expected), Got: $result" +} + +# ============================================================ +# Test: Find-MergeBase function +# ============================================================ +Test-Section "Find-MergeBase Function" + +try { + $baseInfo = Find-MergeBase + + Write-TestResult "Find-MergeBase returns result" ($null -ne $baseInfo) + + if ($baseInfo) { + Write-TestResult "Result has MergeBase property" ($null -ne $baseInfo.MergeBase) + Write-TestResult "MergeBase is valid git commit" ($baseInfo.MergeBase -match '^[a-f0-9]{40}$') + Write-TestResult "Result has BaseBranch property" ($null -ne $baseInfo.BaseBranch) + Write-TestResult "Result has Source property" ($null -ne $baseInfo.Source) + } +} catch { + Write-TestResult "Find-MergeBase executes without error" $false $_.Exception.Message +} + +# ============================================================ +# Test: Get-FixFiles function +# ============================================================ +Test-Section "Get-FixFiles Function" + +try { + $baseInfo = Find-MergeBase + if ($baseInfo) { + $fixFiles = Get-FixFiles -MergeBase $baseInfo.MergeBase -ExplicitFixFiles @() + + Write-TestResult "Get-FixFiles returns array" ($fixFiles -is [array] -or $fixFiles -is [string[]] -or $null -eq $fixFiles) + + if ($fixFiles -and $fixFiles.Count -gt 0) { + # Verify none are test files + $hasTestFile = $false + foreach ($file in $fixFiles) { + if (Test-IsTestFile $file) { + $hasTestFile = $true + break + } + } + Write-TestResult "No test files in fix files list" (-not $hasTestFile) + } else { + Write-TestSkipped "Fix files validation" "No fix files detected (this may be expected)" + } + } +} catch { + Write-TestResult "Get-FixFiles executes without error" $false $_.Exception.Message +} + +# ============================================================ +# Test: Get-FileCategories function +# ============================================================ +Test-Section "Get-FileCategories Function" + +try { + $baseInfo = Find-MergeBase + if ($baseInfo) { + $fixFiles = Get-FixFiles -MergeBase $baseInfo.MergeBase -ExplicitFixFiles @() + + if ($fixFiles -and $fixFiles.Count -gt 0) { + $categories = Get-FileCategories -MergeBase $baseInfo.MergeBase -FixFiles $fixFiles + + Write-TestResult "Get-FileCategories returns result" ($null -ne $categories) + Write-TestResult "Result has RevertableFiles property" ($null -ne $categories.RevertableFiles) + Write-TestResult "Result has NewFiles property" ($null -ne $categories.NewFiles) + + # Total should equal input + $totalCategorized = $categories.RevertableFiles.Count + $categories.NewFiles.Count + Write-TestResult "All files categorized" ($totalCategorized -eq $fixFiles.Count) + } else { + Write-TestSkipped "File categorization validation" "No fix files to categorize" + } + } +} catch { + Write-TestResult "Get-FileCategories executes without error" $false $_.Exception.Message +} + +# ============================================================ +# Test: Script DryRun mode (via subprocess) +# ============================================================ +Test-Section "Script DryRun Mode" + +try { + # Capture current git status + $beforeStatus = git status --porcelain 2>$null + + # Run DryRun and capture output + $output = pwsh -NoProfile -File $ScriptPath -DryRun 2>&1 + $exitCode = $LASTEXITCODE + + # Check git status unchanged + $afterStatus = git status --porcelain 2>$null + + Write-TestResult "DryRun doesn't modify files" ($beforeStatus -eq $afterStatus) + Write-TestResult "DryRun outputs expected text" (($output | Out-String) -match "DRY RUN") + + # Verify no state file created + Write-TestResult "DryRun doesn't create state file" (-not (Test-Path $StateFile)) +} catch { + Write-TestResult "DryRun mode works" $false $_.Exception.Message +} + +# ============================================================ +# Test: Full establish/restore cycle (via subprocess) +# ============================================================ +Test-Section "Establish/Restore Cycle (Script Invocation)" + +# Only run this if we have fix files to work with +try { + $baseInfo = Find-MergeBase + $fixFiles = Get-FixFiles -MergeBase $baseInfo.MergeBase -ExplicitFixFiles @() + $categories = Get-FileCategories -MergeBase $baseInfo.MergeBase -FixFiles $fixFiles + + if ($categories.RevertableFiles.Count -gt 0) { + # Ensure working directory is clean before testing (checkout files to HEAD) + foreach ($file in $categories.RevertableFiles) { + git checkout HEAD -- $file 2>$null + } + + # Save original state (should be empty now) + $originalStatus = git status --porcelain 2>$null + + # Establish baseline + $establishOutput = pwsh -NoProfile -File $ScriptPath 2>&1 + $establishExitCode = $LASTEXITCODE + + $establishOutputStr = $establishOutput | Out-String + Write-TestResult "Establish script executes" ($establishExitCode -eq 0 -or $establishOutputStr -match "Baseline established") + Write-TestResult "State file created" (Test-Path $StateFile) + + # Check files were reverted + $afterEstablish = git status --porcelain 2>$null + $statusChanged = (($afterEstablish | Out-String).Trim()) -ne (($originalStatus | Out-String).Trim()) + Write-TestResult "Files modified after establish" $statusChanged + + # Verify state file has expected structure + if (Test-Path $StateFile) { + $stateContent = Get-Content $StateFile -Raw | ConvertFrom-Json + Write-TestResult "State file has MergeBase" ($null -ne $stateContent.MergeBase) + Write-TestResult "State file has RevertedFiles" ($null -ne $stateContent.RevertedFiles) + } + + # Restore + $restoreOutput = pwsh -NoProfile -File $ScriptPath -Restore 2>&1 + $restoreOutputStr = $restoreOutput | Out-String + + Write-TestResult "Restore outputs expected text" ($restoreOutputStr -match "Baseline restored" -or $restoreOutputStr -match "Restoring") + Write-TestResult "State file removed after restore" (-not (Test-Path $StateFile)) + + # Check clean state + $afterRestore = git status --porcelain 2>$null + $statusRestored = (($afterRestore | Out-String).Trim()) -eq (($originalStatus | Out-String).Trim()) + Write-TestResult "Clean state after restore" $statusRestored + + } else { + Write-TestSkipped "Establish/Restore cycle" "No revertable files available" + } +} catch { + Write-TestResult "Establish/Restore cycle works" $false $_.Exception.Message + # Cleanup on error + Invoke-Cleanup +} + +# ============================================================ +# Test: Restore with no state +# ============================================================ +Test-Section "Restore With No State" + +try { + # Ensure no state file + if (Test-Path $StateFile) { + Remove-Item $StateFile -Force + } + + $output = pwsh -NoProfile -File $ScriptPath -Restore 2>&1 + $outputStr = $output | Out-String + + Write-TestResult "Restore without state completes" ($outputStr -match "No baseline state found" -or $outputStr -match "Nothing to restore") +} catch { + Write-TestResult "Restore without state handles gracefully" $false $_.Exception.Message +} + +# ============================================================ +# Test: Explicit FixFiles parameter +# ============================================================ +Test-Section "Explicit FixFiles Parameter" + +try { + # Pass explicit files + $testFile = "src/Controls/src/Core/Platform/Android/InnerGestureListener.cs" + $output = pwsh -NoProfile -File $ScriptPath -DryRun -FixFiles $testFile 2>&1 + $outputStr = $output | Out-String + + Write-TestResult "Explicit FixFiles accepted" ($outputStr -match "DRY RUN" -or $outputStr -match "revert") + Write-TestResult "Explicit file appears in output" ($outputStr -match "InnerGestureListener") +} catch { + Write-TestResult "Explicit FixFiles works" $false $_.Exception.Message +} + +# ============================================================ +# Test: Explicit BaseBranch parameter +# ============================================================ +Test-Section "Explicit BaseBranch Parameter" + +try { + $output = pwsh -NoProfile -File $ScriptPath -DryRun -BaseBranch "main" 2>&1 + $outputStr = $output | Out-String + + Write-TestResult "Explicit BaseBranch accepted" ($outputStr -match "main" -or $outputStr -match "DRY RUN") +} catch { + Write-TestResult "Explicit BaseBranch works" $false $_.Exception.Message +} + +# ============================================================ +# Test: State file functions +# ============================================================ +Test-Section "State File Functions" + +try { + # Test Save-BaselineState + $testState = @{ + MergeBase = "abc123" + BaseBranch = "test" + RevertedFiles = @("file1.cs", "file2.cs") + NewFiles = @() + Timestamp = (Get-Date -Format "o") + } + + Save-BaselineState $testState + Write-TestResult "Save-BaselineState creates file" (Test-Path $StateFile) + + # Test Get-BaselineState + $loadedState = Get-BaselineState + Write-TestResult "Get-BaselineState returns data" ($null -ne $loadedState) + Write-TestResult "Loaded state has correct MergeBase" ($loadedState.MergeBase -eq "abc123") + Write-TestResult "Loaded state has correct RevertedFiles count" ($loadedState.RevertedFiles.Count -eq 2) + + # Test Remove-BaselineState + Remove-BaselineState + Write-TestResult "Remove-BaselineState removes file" (-not (Test-Path $StateFile)) + +} catch { + Write-TestResult "State file functions work" $false $_.Exception.Message +} finally { + # Cleanup + if (Test-Path $StateFile) { + Remove-Item $StateFile -Force -ErrorAction SilentlyContinue + } +} + +# ============================================================ +# Summary +# ============================================================ +Write-Host "" +Write-Host "============================================" -ForegroundColor Cyan +Write-Host "TEST SUMMARY" -ForegroundColor Cyan +Write-Host "============================================" -ForegroundColor Cyan +Write-Host "Passed: $script:TestsPassed" -ForegroundColor Green +Write-Host "Failed: $script:TestsFailed" -ForegroundColor $(if ($script:TestsFailed -gt 0) { "Red" } else { "Green" }) +Write-Host "Skipped: $script:TestsSkipped" -ForegroundColor Yellow +Write-Host "" + +if ($script:TestsFailed -gt 0) { + Write-Host "TESTS FAILED" -ForegroundColor Red + exit 1 +} else { + Write-Host "ALL TESTS PASSED" -ForegroundColor Green + exit 0 +} diff --git a/.github/skills/find-reviewable-pr/SKILL.md b/.github/skills/find-reviewable-pr/SKILL.md index 6fd6797b88af..2b7f5fcf23a7 100644 --- a/.github/skills/find-reviewable-pr/SKILL.md +++ b/.github/skills/find-reviewable-pr/SKILL.md @@ -133,7 +133,7 @@ Would you like me to review this PR? ### Step 5: Invoke PR Reviewer -When user confirms, use the **pr-reviewer** agent: +When user confirms, use the **pr** agent: - "Review PR #XXXXX" ## Complexity Levels diff --git a/.github/skills/learn-from-pr/SKILL.md b/.github/skills/learn-from-pr/SKILL.md new file mode 100644 index 000000000000..5dbb8dd8be68 --- /dev/null +++ b/.github/skills/learn-from-pr/SKILL.md @@ -0,0 +1,289 @@ +--- +name: learn-from-pr +description: Analyzes a completed PR to extract lessons learned from agent behavior. Use after any PR with agent involvement - whether the agent failed, succeeded slowly, or succeeded quickly. Identifies patterns to reinforce or fix, and generates actionable recommendations for instruction files, skills, and documentation. +metadata: + author: dotnet-maui + version: "1.0" +compatibility: Requires GitHub CLI (gh) +--- + +# Learn From PR + +Extracts lessons learned from a completed PR to improve repository documentation and agent capabilities. + +## Inputs + +| Input | Required | Source | +|-------|----------|--------| +| PR number or Issue number | Yes | User provides (e.g., "PR #33352" or "issue 33352") | +| Session markdown | Optional | `.github/agent-pr-session/issue-XXXXX.md` or `pr-XXXXX.md` | + +## Outputs + +1. **Learning Analysis** - Structured markdown with: + - What happened (problem, attempts, solution) + - Fix location analysis (attempted vs actual) + - Failure modes identified + - Prioritized recommendations + +2. **Actionable Recommendations** - Each with: + - Category, Priority, Location, Specific Change, Why It Helps + +## Completion Criteria + +The skill is complete when you have: +- [ ] Gathered PR diff and metadata +- [ ] Analyzed fix location (attempted vs actual) +- [ ] Identified failure modes +- [ ] Generated at least one concrete recommendation +- [ ] Presented findings to user + +## When to Use + +- After agent failed to find the right fix +- After agent succeeded but took many attempts +- After agent succeeded quickly (to understand what worked) +- When asked "what can we learn from PR #XXXXX?" + +## When NOT to Use + +- Before PR is finalized (use `pr-finalize` first) +- For trivial PRs (typo fixes, simple changes) +- When no agent was involved (nothing to analyze) + +--- + +## Workflow + +### Step 1: Gather Data + +```bash +# Required: Get PR info +gh pr view XXXXX --json title,body,files +gh pr diff XXXXX + +# Check for session markdown +ls .github/agent-pr-session/issue-XXXXX.md .github/agent-pr-session/pr-XXXXX.md 2>/dev/null +``` + +**If session markdown exists, extract:** +- Fix Candidates table (what was tried) +- Files each attempt targeted +- Why attempts failed + +**Analyzing without session markdown:** + +When no session file exists, you can still learn from: +1. **PR discussion** - Comments reveal what was tried +2. **Commit history** - Multiple commits may show iteration +3. **Code complexity** - Non-obvious fixes suggest learning opportunities +4. **Similar past issues** - Search for related bugs + +Focus on: "What would have helped an agent find this fix faster?" + +### Step 2: Fix Location Analysis + +**Critical question:** Did agent attempts target the same files as the final fix? + +```bash +# Where did final fix go? +gh pr view XXXXX --json files --jq '.files[].path' | grep -v test + +# If session markdown exists, compare to attempted files +``` + +| Scenario | Implication | +|----------|-------------| +| Same files | Agent found right location | +| Different files | **Major learning opportunity** - document why | + +**If different files:** Answer these questions: +- Why did agent think that was the right file? +- What search would have found the correct file? + +### Step 3: Analyze Outcome + +Determine which scenario applies and look for the relevant patterns: + +#### Scenario A: Agent Failed + +| Pattern | Indicator | +|---------|-----------| +| **Wrong file entirely** | All attempts in File A, fix in File B | +| **Tunnel vision** | Only looked at file mentioned in error | +| **Trusted issue title** | Issue said "crash in X" so only looked at X | +| **Pattern not generalized** | Fixed one instance, missed others | +| **Didn't search codebase** | Never found similar code patterns | +| **Missing platform knowledge** | Didn't know iOS/Android/Windows specifics | +| **Wrong abstraction layer** | Fixed handler when problem was in core | +| **Misread error message** | Error pointed to symptom, not cause | +| **Incomplete context** | Didn't read enough surrounding code | +| **Over-engineered** | Complex fix when simple one existed | + +#### Scenario B: Agent Succeeded Slowly (many attempts) + +| Pattern | Indicator | +|---------|-----------| +| **Correct file, wrong approach** | Found right file but tried wrong fixes first | +| **Needed multiple iterations** | Each attempt got closer but wasn't quite right | +| **Discovery was slow** | Eventually found it but search was inefficient | +| **Missing domain knowledge** | Had to learn something that could be documented | + +**Key question:** What would have gotten agent to the solution faster? + +#### Scenario C: Agent Succeeded Quickly + +| Pattern | Indicator | +|---------|-----------| +| **Good search strategy** | Found right file immediately | +| **Understood the pattern** | Recognized similar issues from past | +| **Documentation helped** | Existing docs pointed to solution | +| **Simple, minimal fix** | Didn't over-engineer | + +**Key question:** What made this work? Should we reinforce this pattern? + +### Step 4: Find Improvement Locations + +```bash +# Discover where agent guidance lives +find .github/instructions -name "*.instructions.md" 2>/dev/null +find .github/skills -name "SKILL.md" 2>/dev/null +ls docs/design/ 2>/dev/null +ls .github/copilot-instructions.md 2>/dev/null +``` + +| Location | When to Add Here | +|----------|------------------| +| `.github/instructions/*.instructions.md` | Domain-specific AI guidance (testing patterns, platform rules) | +| `.github/skills/*/SKILL.md` | Skill needs new step, checklist, or improved workflow | +| `/docs/design/*.md` | Detailed architectural documentation | +| `.github/copilot-instructions.md` | General AI workflow guidance | +| Code comments | Non-obvious code behavior | + +### Step 5: Generate Recommendations + +For each recommendation, provide: + +1. **Category:** Instruction file / Skill / Architecture doc / Inline comment / Linting issue +2. **Priority:** High (prevents class of bugs) / Medium (helps discovery) / Low (nice to have) +3. **Location:** Exact file path +4. **Specific Change:** Exact text to add +5. **Why It Helps:** Which failure mode it prevents + +**Prioritization factors:** +- How common is this pattern? +- Would future agents definitely hit this again? +- How hard is it to implement? + +**Pattern-to-Improvement Mapping (Failures/Slow Success):** + +| Pattern | Likely Improvement | +|---------|-------------------| +| Wrong file entirely | Check `/docs/design/` for component relationships | +| Tunnel vision | Instruction file: "Always search for pattern across codebase" | +| Missing platform knowledge | Platform-specific instruction file | +| Wrong abstraction layer | Reference `/docs/design/HandlerResolution.md` | +| Misread error message | Code comment explaining the real cause | +| Over-engineered | Skill enhancement: "Try simplest fix first" | + +**Pattern-to-Improvement Mapping (Quick Success - reinforce):** + +| Pattern | Improvement | +|---------|-------------| +| Good search strategy | Document the search pattern that worked in skills | +| Documentation helped | Note which docs were valuable, ensure they stay updated | +| Recognized pattern | Add to instruction files as known pattern | + +### Step 6: Present Findings + +Present your analysis covering: +- What happened and what made it hard +- Where agent looked vs actual fix location +- Which patterns applied and evidence +- Prioritized recommendations with full details (category, priority, location, exact change, why it helps) + +--- + +## Error Handling + +| Situation | Action | +|-----------|--------| +| PR not found | Ask user to verify PR number | +| No session markdown | Analyze PR diff only, note limited context | +| No agent involvement evident | Ask user if they still want analysis | +| Can't determine failure mode | State "insufficient data" and what's missing | + +## Constraints + +- **Analysis only** - Don't apply changes (use learn-from-pr agent for that) +- **Actionable recommendations** - Every recommendation must have specific file path and text +- **Don't duplicate** - Check existing docs before recommending new ones +- **Focus on high-value learning** - Skip trivial observations +- **Respect PR scope** - Don't recommend improvements unrelated to the PR's learnings + +--- + +## Examples + +### Example: Wrong File Entirely + +**PR #33352** - TraitCollectionDidChange crash on MacCatalyst + +**What happened:** +- Issue title: "ObjectDisposedException in ShellSectionRootRenderer" +- Agent made 11 attempts, ALL in `ShellSectionRootRenderer.cs` +- Actual fix was in `PageViewController.cs` + +**Failure Mode:** Trusted issue title instead of searching for pattern. + +**Recommendation:** +- **Category:** Instruction File +- **Location:** `.github/instructions/ios-debugging.instructions.md` +- **Change:** "When fixing iOS crashes, search for the PATTERN across all files, not just the file named in the error" +- **Why:** Prevents tunnel vision on named file + +### Example: Slow Success + +**PR #34567** - CollectionView scroll position not preserved + +**What happened:** +- Agent took 5 attempts to find fix +- First 3 attempts were in wrong layer (handler vs core) +- Eventually found it after reading more context +- Final fix was simple once the right layer was identified + +**Pattern:** Wrong abstraction layer - fixed handler when problem was in core. + +**Recommendation:** +- **Category:** Architecture Doc +- **Location:** `.github/architecture/handler-vs-core.md` +- **Change:** Document layer responsibilities - handlers map properties, core handles behavior +- **Why:** Helps agent identify correct layer faster + +### Example: Quick Success + +**PR #35678** - Button disabled state not updating + +**What happened:** +- Agent found fix in 1 attempt +- Searched for "IsEnabled" pattern across codebase immediately +- Found similar past fix in another control and applied same approach +- Simple, minimal change + +**Pattern:** Good search strategy - recognized pattern from similar code. + +**Recommendation:** +- **Category:** Skill Enhancement +- **Location:** `.github/skills/try-fix/SKILL.md` +- **Change:** Add to search strategy: "Search for same property pattern in other controls" +- **Why:** Reinforces successful discovery technique + +--- + +## Integration + +- **pr-finalize** → Use first to verify PR is ready +- **learn-from-pr skill** → Analysis only (this skill) +- **learn-from-pr agent** → Analysis + apply changes + +For automated application of recommendations, use the `learn-from-pr` agent instead. diff --git a/.github/skills/pr-finalize/SKILL.md b/.github/skills/pr-finalize/SKILL.md index c125583f8ee6..b30328c6ce4d 100644 --- a/.github/skills/pr-finalize/SKILL.md +++ b/.github/skills/pr-finalize/SKILL.md @@ -1,6 +1,9 @@ --- name: pr-finalize -description: Finalizes any PR for merge by verifying title and description match actual implementation. Ensures commit message helps future agents understand the change. Use on any PR before merge, when description may be stale, or to review commit message quality. +description: Verifies PR title and description match actual implementation. Works on any PR. Optionally updates agent session markdown if present. +metadata: + author: dotnet-maui + version: "1.0" compatibility: Requires GitHub CLI (gh) --- @@ -8,52 +11,165 @@ compatibility: Requires GitHub CLI (gh) Ensures PR title and description accurately reflect the implementation for a good commit message. -**Standalone skill** - Can be used on any PR, not just PRs created by the pr agent. +**Works on any PR** - Does not require agent involvement or session markdown. + +## Inputs + +| Input | Required | Source | +|-------|----------|--------| +| PR number | Yes | User provides | +| Session markdown | Optional | `.github/agent-pr-session/issue-XXXXX.md` or `pr-XXXXX.md` | + +## Outputs + +1. **Recommended PR title** - Platform prefix, behavior-focused +2. **Recommended PR description** - NOTE block + structured content +3. **Session markdown updates** (only if file exists) + +## Completion Criteria + +- [ ] Reviewed PR diff to understand actual implementation +- [ ] Verified title matches implementation (or recommended fix) +- [ ] Verified description matches implementation (or recommended fix) +- [ ] Checked for session markdown and updated if needed ## When to Use -- "Finalize PR #XXXXX" -- "Check PR description for #XXXXX" -- "Review commit message for PR #XXXXX" - Before merging any PR -- When PR implementation changed during review +- "Finalize PR #XXXXX" +- "Check PR description for #XXXXX" +- When implementation changed during review -## Usage +## When NOT to Use -```bash -# Get current state (no local checkout required) -gh pr view XXXXX --json title,body -gh pr view XXXXX --json files --jq '.files[].path' +- ❌ For analyzing lessons learned (use `learn-from-pr` skill instead) +- ❌ For writing or running tests (use `write-tests` or sandbox) +- ❌ For investigating why PR build failed (use `pr-build-status`) -# Review commit messages (helpful for squash/merge commit quality) -gh pr view XXXXX --json commits --jq '.commits[].messageHeadline' +## Constraints -# Review actual code changes -gh pr diff XXXXX +- **Don't change PR title/description directly** - Only recommend changes +- **Don't modify code** - Only verify title/description accuracy +- **Match existing style** - Follow PR template format +- **Preserve user content** - Enhance, don't replace custom descriptions + +--- -# Optional: if the PR branch is checked out locally -git diff origin/main...HEAD +## Workflow + +### Step 1: Get PR State + +```bash +gh pr view XXXXX --json title,body,files +gh pr diff XXXXX +gh pr view XXXXX --json commits --jq '.commits[].messageHeadline' ``` -Then produce: -- Recommended PR title -- Recommended PR description (including the required NOTE block) -- Optional: suggestions to improve commit message quality (usually align PR title/body with the intended squash commit title/body) +### Step 2: Analyze Implementation -## Title Requirements +Read the diff and understand: +- What was actually changed (not what was planned) +- Which platforms are affected +- What the fix does + +### Step 3: Verify Title | Requirement | Good | Bad | |-------------|------|-----| | Platform prefix (if specific) | `[iOS] Fix Shell back button` | `Fix Shell back button` | -| Describes behavior, not issue | `Fix long-press not triggering events` | `Fix #23892` | +| Describes behavior | `Fix long-press not triggering events` | `Fix #23892` | | No noise prefixes | `[iOS] Fix...` | `[PR agent] Fix...` | +### Step 4: Verify Description + +Must include: +1. NOTE block (for testing PR artifacts) +2. Description of Change (matches actual implementation) +3. Issues Fixed + +Should include (for agent context): +- Root cause +- Fix approach +- Key insight + +### Step 5: Session Markdown (If Exists) + +```bash +ls .github/agent-pr-session/issue-XXXXX.md .github/agent-pr-session/pr-XXXXX.md 2>/dev/null +``` + +**If file exists:** +- Check if "Selected Fix: [PENDING]" → update with actual fix +- Add "ACTUAL IMPLEMENTED FIX" section if missing +- Document lessons learned + +**If file doesn't exist:** Skip this step. + +### Step 6: Present Recommendations + +Output recommended title and description. + +--- + +## Error Handling + +| Situation | Action | +|-----------|--------| +| PR not found | Ask user to verify PR number | +| No session markdown | Proceed - only verify title/description | +| Title already good | Confirm it's good, no change needed | +| Description missing | Generate recommended description | + +--- + +## Output Template + +```markdown +# PR Finalize: #XXXXX + +## Title Assessment +**Current:** [current title] +**Recommendation:** [recommended title, or "✅ Current title is good"] + +## Description Assessment +**Issues:** +- [issue 1] +- [issue 2] + +**Recommended Description:** + + +> [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! + +### Description of Change + +[Brief summary matching implementation] + +**Root cause:** [Why bug occurred] + +**Fix:** [What code now does] + +### Issues Fixed + +Fixes #XXXXX + +## Session Markdown +[Updated / No file exists / Already complete] +``` + +--- + +## Title Requirements + +- **Platform prefix** if platform-specific: `[iOS]`, `[Android]`, `[Windows]`, `[MacCatalyst]` +- **Behavior-focused** - Describe what changed, not issue number +- **Concise** - Should fit in commit message subject line + ## Description Requirements -PR description should: -1. Start with the required NOTE block (so users can test PR artifacts) -2. Include the base sections from `.github/PULL_REQUEST_TEMPLATE.md` ("Description of Change" and "Issues Fixed"). The skill adds additional structured fields (Root cause, Fix, Key insight, etc.) as recommended enhancements for better agent context. -3. Match the actual implementation +### Required Sections ```markdown @@ -68,63 +184,102 @@ PR description should: Fixes #XXXXX ``` -## Content for Future Agents - -Add these elements so future agents can understand the change: +### Recommended Additions (for agent context) | Element | Purpose | |---------|---------| | **Root cause** | Why the bug occurred | | **Fix approach** | What the code now does | -| **Key insight** | Non-obvious understanding that made fix work | -| **What to avoid** | Patterns that would re-break it | -| **Regression chain** | PRs that caused/affected this (if applicable) | -| **Related issues** | Issues verified not to regress | +| **Key insight** | Non-obvious understanding | +| **What to avoid** | Patterns that would re-break | -## Common Issues +--- -| Problem | Cause | Solution | -|---------|-------|----------| -| Description doesn't match code | Implementation changed during review | Rewrite description from actual diff | -| Missing root cause | Author focused on "what" not "why" | Add root cause from issue/analysis | -| References wrong approach | Started with A, switched to B | Update to describe final approach | +## Session Markdown Updates (Optional) -## Output Template +If session markdown exists and is incomplete, add: ```markdown -# Recommended PR Title +## ACTUAL IMPLEMENTED FIX -[Platform] Brief description of behavior fix +**Selected Fix:** [Brief description] + +**What was implemented:** +1. [What changed] +2. [Key files] + +**Key insight:** [Non-obvious understanding] + +## Lessons Learned + +**What would have helped:** +1. [Suggestion 1] +2. [Suggestion 2] +``` --- -# Recommended PR Description +## Common Issues - -> [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! +| Problem | Solution | +|---------|----------| +| Description doesn't match code | Rewrite from actual diff | +| Missing root cause | Add from issue/analysis | +| References wrong approach | Update to describe final approach | +| "Selected Fix: [PENDING]" | Document actual implementation | -### Description of Change +--- -[Brief summary] +## Example -**Root cause:** [Why bug occurred] +**PR #33352** - MacCatalyst crash on window disposal -**Fix:** [What code now does] +**Step 1: Get PR State** +```bash +gh pr view 33352 --json title,body,files +# Title: "Fix ObjectDisposedException in PageViewController" +# Files: PageViewController.cs +``` -**Key insight:** [Non-obvious understanding] +**Step 2: Analyze Implementation** +- Fix adds null check for window.Handler before accessing services +- Platform: MacCatalyst specific +- Prevents crash during TraitCollectionDidChange when window disposed -**Regression chain:** (if applicable) -| PR | What happened | -|-----|---------------| -| #XXXXX | Caused regression | +**Step 3: Verify Title** +- Current: "Fix ObjectDisposedException in PageViewController" +- Assessment: Missing platform prefix +- Recommended: `[MacCatalyst] Fix ObjectDisposedException in PageViewController.TraitCollectionDidChange` -**What to avoid:** [Patterns that would re-break] +**Step 4: Verify Description** +- Missing NOTE block +- Missing root cause explanation +- Generate recommended description -### Issues Fixed +**Output:** +```markdown +# PR Finalize: #33352 -Fixes #XXXXX +## Title Assessment +**Current:** Fix ObjectDisposedException in PageViewController +**Recommendation:** [MacCatalyst] Fix ObjectDisposedException in PageViewController.TraitCollectionDidChange -**Related:** #YYYYY (verified not regressed ✅) +## Description Assessment +**Issues:** +- Missing NOTE block for testing PR artifacts +- Missing root cause explanation + +**Recommended Description:** +[Full description with NOTE block, root cause, fix approach...] + +## Session Markdown +Updated - Added "ACTUAL IMPLEMENTED FIX" section ``` + +--- + +## Integration + +- **pr-finalize** → Verify PR ready to merge (this skill) +- **learn-from-pr** → Extract lessons after finalization +- **learn-from-pr agent** → Extract lessons AND apply improvements diff --git a/.github/skills/try-fix/SKILL.md b/.github/skills/try-fix/SKILL.md index c266f3e1dabc..eeaf4c9b1ad8 100644 --- a/.github/skills/try-fix/SKILL.md +++ b/.github/skills/try-fix/SKILL.md @@ -1,242 +1,291 @@ --- name: try-fix -description: Proposes ONE independent fix approach, applies it, runs tests, records result with failure analysis in state file, then reverts. Reads prior attempts to learn from failures. Returns exhausted=true when no more ideas. Max 5 attempts per session. +description: Attempts ONE fix for a provided problem, tests it, and reports results. Use when CI or an agent needs to try fixing a bug with full context provided upfront. Invoke with problem description, test command, target files, and optional hints. +metadata: + author: dotnet-maui + version: "2.0" +compatibility: Requires git, PowerShell, and .NET SDK for building and running tests. --- # Try Fix Skill -Proposes and tests ONE independent fix approach per invocation. The agent invokes this skill repeatedly to explore multiple alternatives. +Attempts ONE fix for a given problem. Receives all context upfront, tries a single approach, tests it, and reports what happened. ## Core Principles -1. **Single-shot**: Each invocation = ONE fix idea, tested, recorded, reverted -2. **Independent**: Generate fix ideas WITHOUT looking at or being influenced by the PR's fix +1. **Single-shot**: Each invocation = ONE fix idea, tested, reported +2. **Context-driven**: All necessary information provided upfront by invoker 3. **Empirical**: Actually implement and test - don't just theorize -4. **Learning**: When a fix fails, analyze WHY and record the flawed reasoning +4. **Informative**: Report what was tried, what happened, and why ## When to Use -- ✅ After Gate passes - you have a verified reproduction test -- ✅ When exploring independent fix alternatives (even if PR already has a fix) -- ✅ When the agent needs to iterate through multiple fix attempts +- ✅ CI automation needs to attempt a fix +- ✅ You have a clear problem description and test command +- ✅ You want ONE attempt with full reporting ## When NOT to Use -- ❌ Before Gate passes (you need a test that catches the bug first) - ❌ For writing tests (use `write-tests` skill) -- ❌ For just running tests (use `BuildAndRunHostApp.ps1` directly) -- ❌ To test the PR's existing fix (Gate already validated that) +- ❌ For just running tests (use test commands directly) +- ❌ When problem context is unclear (gather context first) --- ## Inputs -Before invoking this skill, ensure you have: +All inputs are provided by the invoker (CI, agent, or user). -| Input | Source | Example | -|-------|--------|---------| -| State file path | Agent workflow | `.github/agent-pr-session/pr-12345.md` | -| Test filter | From test files | `Issue12345` | -| Platform | From issue labels | `android` or `ios` | -| PR fix files | From Pre-Flight | Files changed by PR (to revert) | +| Input | Required | Description | +|-------|----------|-------------| +| Problem | Yes | Description of the bug/issue to fix | +| Test command | Yes | Command to verify if fix works (e.g., `pwsh BuildAndRunHostApp.ps1 -Platform android -TestFilter "Issue12345"`) | +| Target files | Yes | Files to investigate for the fix | +| Platform | Yes | Target platform (`android`, `ios`, `windows`, `maccatalyst`) | +| Hints | Optional | Suggested approaches, prior attempts, or areas to focus on | +| Baseline | Optional | Git ref or instructions for establishing broken state (default: current state) | +| state_file | Optional | Path to PR agent state file (e.g., `.github/agent-pr-session/pr-12345.md`). If provided, try-fix will append its results to the Fix Candidates table. | ---- +## Outputs -## Workflow +Results reported back to the invoker: -### Step 1: Read State File and Learn from Prior Attempts +| Field | Description | +|-------|-------------| +| `approach` | What fix was attempted (brief description) | +| `files_changed` | Which files were modified | +| `result` | `PASS` or `FAIL` | +| `analysis` | Why it worked, or why it failed and what was learned | +| `diff` | The actual code changes made (for review) | -Read the state file to find prior attempts: +## Completion Criteria -```bash -cat .github/agent-pr-session/pr-XXXXX.md -``` +The skill is complete when: +- [ ] Problem understood from provided context +- [ ] ONE fix approach designed and implemented +- [ ] Tests run and result captured +- [ ] Analysis provided (success explanation or failure reasoning) +- [ ] Results reported to invoker +- [ ] Working directory restored to original state -Look for the **Fix Candidates** table. For each prior attempt: -- What approach was tried? -- Did it pass or fail? -- **If it failed, WHY did it fail?** (This is critical for learning) +--- -**Use failure analysis to avoid repeating mistakes:** -- If attempt #1 failed because "too late in lifecycle" → don't try other late-lifecycle fixes -- If attempt #2 failed because "trigger wasn't enough, calculation logic needed fixing" → focus on calculation logic +## Workflow -### Step 2: Revert PR's Fix (Get Broken Baseline) +### Step 1: Understand the Problem -**🚨 CRITICAL: You must work from a broken state where the bug exists.** +Review the provided context: +- What is the bug/issue? +- What test command verifies the fix? +- What files should be investigated? +- Are there hints about what to try or avoid? -```bash -# Identify the PR's fix files from the state file "Files Changed" section -# Revert ALL fix files (not test files) -git checkout HEAD~1 -- src/path/to/fix1.cs src/path/to/fix2.cs +**If state_file provided, review prior attempts:** +1. Read the Fix Candidates table +2. Note which approaches failed and WHY (the Notes column) +3. Avoid repeating failed approaches +4. Build on partial successes (if an approach was "close", try a variation) +5. Use failure analysis to inform your approach -# Verify the bug is present (test should FAIL) -# This is your baseline -``` +**Do NOT search for additional context.** Work with what's provided. -**Why?** You're testing whether YOUR fix works, independent of the PR's fix. +### Step 2: Establish Baseline (if specified) -### Step 3: Check if Exhausted +Use the shared baseline script to revert fix files while preserving tests: -Before proposing a new fix, evaluate: +```bash +# Establish baseline - reverts fix files to merge-base state +$baseline = pwsh .github/scripts/EstablishBrokenBaseline.ps1 -1. **Count prior try-fix attempts** - If 5+ attempts already recorded, return `exhausted=true` -2. **Review what's been tried and WHY it failed** - Can you think of a meaningfully different approach? -3. **If no new ideas** - Return `exhausted=true` +# Or with explicit options: +$baseline = pwsh .github/scripts/EstablishBrokenBaseline.ps1 -BaseBranch main +$baseline = pwsh .github/scripts/EstablishBrokenBaseline.ps1 -FixFiles @("src/path/to/file.cs") +$baseline = pwsh .github/scripts/EstablishBrokenBaseline.ps1 -DryRun # Preview without changes +``` -**Signs you're exhausted:** -- All obvious approaches have been tried -- Remaining ideas are variations of failed attempts (same root flaw) -- You keep coming back to approaches similar to what failed -- The problem requires architectural changes beyond scope +The script: +- Auto-detects merge-base from PR metadata or common branch patterns +- Identifies fix files (non-test files that changed since merge-base) +- Reverts only files that existed at merge-base (preserves new files) +- Saves state for `-Restore` to undo later -If exhausted, **stop here** and return to the agent with `exhausted=true`. +**CRITICAL:** Remember to restore in Step 9. If something fails mid-fix, run: +```bash +pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore +``` -### Step 4: Analyze the Code (Independent of PR's Fix) +If no baseline specified, work from current state. -**🚨 DO NOT look at the PR's fix implementation.** Generate your own ideas. +### Step 3: Analyze Target Files -Research the bug to propose a NEW approach: +Read the target files to understand the code: ```bash -# Find the affected code -grep -r "SymptomOrClassName" src/Controls/src/ --include="*.cs" -l - -# Look at the implementation -cat path/to/affected/File.cs - -# Check git history for context (but NOT the PR's commits) -git log --oneline -10 -- path/to/affected/File.cs +# Read the files specified in inputs +cat src/path/to/TargetFile.cs ``` **Key questions:** - What is the root cause of this bug? -- Where in the code should a fix go? +- Where should the fix go? - What's the minimal change needed? -- How is this different from prior failed attempts? -### Step 5: Propose ONE Fix +### Step 4: Design ONE Fix -Design an approach that is: -- **Independent** - NOT influenced by the PR's solution -- **Different** from prior attempts in the state file -- **Informed** by WHY prior attempts failed -- **Minimal** - smallest change that fixes the issue +Based on your analysis and any provided hints, design a single fix approach: -Document your approach before implementing: - Which file(s) to change - What the change is - Why you think this will work -- How it differs from prior failed attempts -### Step 6: Apply the Fix +**If hints suggest specific approaches**, prioritize those. -Edit the necessary files to implement your fix. +### Step 5: Apply the Fix -**Track which files you modify** - you'll need to revert them later. +Implement your fix. Track what you change: ```bash -# Note the files you're about to change +# Before editing, note current state git status --short + +# Apply your fix +# [edit files] + +# After editing, capture what changed +git diff ``` -### Step 7: Run Tests +### Step 6: Run Tests -Run the reproduction test to see if your fix works: +Run the provided test command: ```bash +# Use the exact test command provided in inputs pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform $PLATFORM -TestFilter "$TEST_FILTER" ``` **Capture the result:** -- ✅ **PASS** - Your fix works (test now passes) -- ❌ **FAIL** - Your fix doesn't work (test still fails, or other tests broke) +- ✅ **PASS** - Fix works (test passes) +- ❌ **FAIL** - Fix doesn't work (test fails or other issues) + +### Step 7: Analyze Result + +**If PASS:** +- Why did this fix work? +- Is it the minimal change needed? +- Any concerns or caveats? -### Step 8: If Failed - Analyze WHY +**If FAIL:** +- What was your hypothesis? +- What actually happened? +- Why was the reasoning flawed? +- What insight does this provide for future attempts? -**🚨 CRITICAL: This step is required for failed attempts.** +### Step 8: Capture Diff -When your fix fails, analyze: +Before reverting, capture the diff for reporting: -1. **What was your hypothesis?** Why did you think this would work? -2. **What actually happened?** What did the test output show? -3. **Why was your reasoning flawed?** What did you misunderstand about the bug? -4. **What would be needed instead?** What insight does this failure provide? +```bash +git diff > /tmp/fix-attempt.diff +# or just include inline in your report +``` -This analysis helps future try-fix invocations avoid the same mistake. +### Step 9: Restore Working Directory -### Step 9: Update State File +Restore the baseline (if established in Step 2) and revert any fix changes: -Add a new row to the **Fix Candidates** table in the state file: +```bash +# Restore files reverted by EstablishBrokenBaseline.ps1 +pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore -**For PASSING fixes:** -```markdown -| # | Source | Approach | Test Result | Files Changed | Model | Notes | -|---|--------|----------|-------------|---------------|-------|-------| -| N | try-fix | [Your approach] | ✅ PASS | `file.cs` (+X) | [Model Name] | Works! [any observations] | +# Revert any other changes made during the fix attempt +git checkout -- . ``` -**For FAILING fixes (include failure analysis):** +**Note:** The `-Restore` flag reads the saved state from Step 2 and restores only the files that were reverted. This ensures a clean return to the original state. + +### Step 10: Report Results + +Provide structured output to the invoker: + ```markdown -| # | Source | Approach | Test Result | Files Changed | Model | Notes | -|---|--------|----------|-------------|---------------|-------|-------| -| N | try-fix | [Your approach] | ❌ FAIL | `file.cs` (+X) | [Model Name] | **Why failed:** [Analysis of flawed reasoning and what you learned] | -``` +## Try-Fix Result -### Step 10: Revert Everything +**Approach:** [Brief description of what was tried] -**Always revert** to restore the PR's original state: +**Files Changed:** +- `path/to/file.cs` (+X/-Y lines) -```bash -# Revert ALL changes (your fix AND the PR revert from Step 2) -git checkout -- . +**Result:** ✅ PASS / ❌ FAIL + +**Analysis:** +[Why it worked, or why it failed and what was learned] + +**Diff:** +```diff +[The actual changes made] ``` -**Do NOT revert the state file** - the new candidate row should persist. +**Exhausted:** Yes/No +**Reasoning:** [Why you believe there are/aren't more viable approaches] +``` -### Step 11: Return to Agent +### Determining Exhaustion -Report back to the agent with: +Before updating the state file, evaluate if you've exhausted viable approaches: -| Field | Value | -|-------|-------| -| `approach` | Brief description of what was tried | -| `test_result` | PASS or FAIL | -| `exhausted` | true if no more ideas, false otherwise | +**Set `exhausted=true` when:** +- You've tried the same fundamental approach multiple times with variations +- All hints have been explored without success +- Failure analysis reveals the problem is outside the target files +- No new ideas remain based on prior failure analyses ---- +**Set `exhausted=false` when:** +- This is the first attempt +- Failure analysis suggests a different approach within target files +- Hints remain unexplored +- The approach was close but needs refinement -## Fix Candidates Table Format +### Step 11: Update State File (if provided) -The state file should have this section: +If `state_file` input was provided and file exists: -```markdown -## Fix Candidates +1. **Read current Fix Candidates table** from state file +2. **Determine next attempt number** (count existing try-fix rows + 1) +3. **Append new row** with this attempt's results: -| # | Source | Approach | Test Result | Files Changed | Model | Notes | -|---|--------|----------|-------------|---------------|-------|-------| -| 1 | try-fix | Fix in TabbedPageManager | ❌ FAIL | `TabbedPageManager.cs` (+5) | Claude 3.5 Sonnet | **Why failed:** Too late in lifecycle - by the time OnPageSelected fires, layout already measured with stale values | -| 2 | try-fix | RequestApplyInsets only | ❌ FAIL | `ToolbarExtensions.cs` (+2) | Claude 3.5 Sonnet | **Why failed:** Trigger alone insufficient - calculation logic still used cached values | -| 3 | try-fix | Reset cache + RequestApplyInsets | ✅ PASS | `ToolbarExtensions.cs`, `InsetListener.cs` (+8) | Claude 3.5 Sonnet | Works! Similar to PR's approach | -| PR | PR #XXXXX | [PR's approach] | ✅ PASS (Gate) | [files] | Author | Original PR - validated by Gate | +| # | Source | Approach | Test Result | Files Changed | Notes | +|---|--------|----------|-------------|---------------|-------| +| N | try-fix #N | [approach] | ✅ PASS / ❌ FAIL | [files] | [analysis] | -**Exhausted:** Yes -**Selected Fix:** #3 or PR - both work, compare for simplicity +4. **Set exhausted status** based on your determination above +5. **Commit state file:** +```bash +git add "$STATE_FILE" && git commit -m "try-fix: attempt #N (exhausted=$EXHAUSTED)" ``` -**Note:** The PR's fix is recorded as reference (validated by Gate) but is NOT tested by try-fix. +**If no state file provided:** Skip this step (results returned to invoker only). ---- +**Ownership rule:** try-fix updates its own row AND the exhausted field. Never modify: +- Phase status fields +- "Selected Fix" field +- Other try-fix rows -## Guidelines for Proposing Fixes +--- -### Independence is Critical +## Error Handling -🚨 **DO NOT look at the PR's fix code when generating ideas.** +| Situation | Action | +|-----------|--------| +| Problem unclear | Report "insufficient context" - specify what's missing | +| Test command fails to run | Report build/setup error with details | +| Test times out | Report timeout, include partial output | +| Can't determine fix approach | Report "no viable approach identified" with reasoning | +| Git state unrecoverable | Run `git checkout -- .` and `git clean -fd` if needed | -The goal is to see if you can independently arrive at the same solution (validating the PR's approach) or find a better alternative. +--- -If your independent fix matches the PR's approach, that's strong validation. If you find a simpler/better approach, that's valuable feedback. +## Guidelines for Proposing Fixes ### Good Fix Approaches @@ -248,94 +297,102 @@ If your independent fix matches the PR's approach, that's strong validation. If ### Approaches to Avoid -❌ **Looking at the PR's fix first** - Generate ideas independently -❌ **Duplicating prior failed attempts** - Check the table and learn from failures -❌ **Variations of failed approaches with same root flaw** - If timing was wrong, a different timing approach is needed ❌ **Massive refactors** - Keep changes minimal ❌ **Suppressing symptoms** - Fix root cause, not symptoms +❌ **Ignoring provided hints** - Hints exist for a reason +❌ **Multiple unrelated changes** - ONE focused fix per invocation -### Learning from Failures +### Failure Analysis Quality -When a fix fails, the failure analysis is crucial: +When a fix fails, analysis quality matters: -**Bad note:** "Didn't work" -**Good note:** "**Why failed:** RequestApplyInsets triggers recalculation, but MeasuredHeight was still cached from previous layout pass. Need to also invalidate the cached measurement." +**Bad:** "Didn't work" -This helps the next try-fix invocation avoid the same mistake. +**Good:** "Fix attempted to reset state in OnPageSelected, but this fires after layout measurement. The cached MeasuredHeight value was already used. A fix needs to invalidate the cache BEFORE the layout pass, not after." --- -## Example Session +## Example Invocation -**State file before (after Gate passed):** -```markdown -## Fix Candidates +**Inputs provided:** +```yaml +problem: | + CollectionView throws ObjectDisposedException when navigating back + from a page with a CollectionView on Android. + +test_command: | + pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform android -TestFilter "Issue54321" -| # | Source | Approach | Test Result | Files Changed | Model | Notes | -|---|--------|----------|-------------|---------------|-------|-------| -| PR | PR #33359 | RequestApplyInsets + reset appBarHasContent | ✅ PASS (Gate) | 2 files | Author | Original PR | +target_files: + - src/Controls/src/Core/Handlers/Items/ItemsViewHandler.Android.cs + - src/Controls/src/Core/Handlers/Items/CollectionViewHandler.Android.cs -**Exhausted:** No -**Selected Fix:** [PENDING] +platform: android + +hints: | + - The issue seems related to disposal timing + - Similar issue was fixed in ListView by checking IsDisposed before accessing adapter + - Focus on the Disconnect/Cleanup methods ``` -**try-fix invocation #1:** -1. Reads state → sees PR's fix passed Gate, no try-fix attempts yet -2. Reverts PR's fix files → now bug exists -3. Analyzes code independently → proposes: "Fix in TabbedPageManager.OnPageSelected" -4. Applies fix → edits `TabbedPageManager.cs` -5. Runs tests → ❌ FAIL -6. Analyzes failure → "Too late in lifecycle, layout already measured" -7. Updates state file → adds try-fix Candidate #1 with failure analysis -8. Reverts everything (including restoring PR's fix) -9. Returns `{approach: "Fix in TabbedPageManager", test_result: FAIL, exhausted: false}` - -**State file after invocation #1:** +**Skill execution:** +1. Reads context - understands it's a disposal timing issue on Android +2. Analyzes target files - finds `DisconnectHandler` method +3. Designs fix - add `IsDisposed` check before accessing adapter +4. Applies fix - edits `CollectionViewHandler.Android.cs` +5. Runs test - ✅ PASS +6. Analyzes - "Adding IsDisposed check prevents access to disposed adapter during navigation" +7. Captures diff +8. Reverts changes +9. Reports results + +**Output:** ```markdown -## Fix Candidates +## Try-Fix Result -| # | Source | Approach | Test Result | Files Changed | Model | Notes | -|---|--------|----------|-------------|---------------|-------|-------| -| 1 | try-fix | Fix in TabbedPageManager.OnPageSelected | ❌ FAIL | `TabbedPageManager.cs` (+5) | Claude 3.5 Sonnet | **Why failed:** Too late in lifecycle - OnPageSelected fires after layout measured | -| PR | PR #33359 | RequestApplyInsets + reset appBarHasContent | ✅ PASS (Gate) | 2 files | Author | Original PR | +**Approach:** Add IsDisposed check in DisconnectHandler before accessing adapter -**Exhausted:** No -**Selected Fix:** [PENDING] -``` +**Files Changed:** +- `src/Controls/src/Core/Handlers/Items/CollectionViewHandler.Android.cs` (+3/-0 lines) -**try-fix invocation #2:** -1. Reads state → sees attempt #1 failed because "too late in lifecycle" -2. Reverts PR's fix → bug exists -3. Learns from #1 → needs earlier timing, proposes: "Trigger in UpdateIsVisible" -4. Applies fix → edits `ToolbarExtensions.cs` -5. Runs tests → ✅ PASS -6. Updates state file → adds Candidate #2 -7. Reverts everything -8. Returns `{approach: "Trigger in UpdateIsVisible", test_result: PASS, exhausted: false}` - -**State file after invocation #2:** -```markdown -## Fix Candidates +**Result:** ✅ PASS -| # | Source | Approach | Test Result | Files Changed | Model | Notes | -|---|--------|----------|-------------|---------------|-------|-------| -| 1 | try-fix | Fix in TabbedPageManager.OnPageSelected | ❌ FAIL | `TabbedPageManager.cs` (+5) | Claude 3.5 Sonnet | **Why failed:** Too late in lifecycle | -| 2 | try-fix | RequestApplyInsets in UpdateIsVisible | ✅ PASS | `ToolbarExtensions.cs` (+2) | Claude 3.5 Sonnet | Works! Simpler than PR (1 file vs 2) | -| PR | PR #33359 | RequestApplyInsets + reset appBarHasContent | ✅ PASS (Gate) | 2 files | Author | Original PR | +**Analysis:** +The ObjectDisposedException occurred because DisconnectHandler was called during +navigation after the handler was already disposed. Adding an early return when +IsDisposed is true prevents the null adapter access. This matches the pattern +used in ListView's fix (as noted in hints). -**Exhausted:** No -**Selected Fix:** [PENDING] +**Diff:** +```diff + protected override void DisconnectHandler(RecyclerView platformView) + { ++ if (IsDisposed) ++ return; ++ + base.DisconnectHandler(platformView); +``` ``` - -**Agent decides:** Found a passing alternative (#2). Can continue to find more, or stop and compare #2 vs PR. --- -## Constraints - -- **Max 5 try-fix attempts** per session (PR's fix is NOT counted - it was validated by Gate) -- **Always revert** after each attempt (restore PR's original state) -- **Always update state file** before reverting -- **Never skip testing** - every fix must be validated empirically -- **Never look at PR's fix** when generating ideas - stay independent -- **Always analyze failures** - record WHY fixes didn't work +## What the Invoker Controls + +This skill does ONE attempt. The invoker (CI pipeline, agent, user) controls: + +| Decision | Invoker's responsibility | +|----------|--------------------------| +| How many attempts | Invoke skill multiple times if needed | +| Max attempts | Configure loop limit (default: 5, can be set higher) | +| Early termination | Stop when try-fix reports `exhausted=true` | +| When to stop | Interpret results and decide (exhausted OR max reached) | +| State file path | Optionally provide for automatic recording | +| Passing context between attempts | Provide updated hints on subsequent calls | +| Success criteria | Evaluate the reported result | +| Phase status | Update phase to COMPLETE when done | +| Final selection | Set "Selected Fix" field when a fix passes | + +The skill records its own results and exhausted status when a state file is provided. The loop should terminate when EITHER: +1. A fix passes (success) +2. `exhausted=true` is reported (no more viable approaches) +3. Max attempts reached (configurable cap) diff --git a/.github/skills/verify-tests-fail-without-fix/SKILL.md b/.github/skills/verify-tests-fail-without-fix/SKILL.md index 37f9a649cf0e..c4529f940cbb 100644 --- a/.github/skills/verify-tests-fail-without-fix/SKILL.md +++ b/.github/skills/verify-tests-fail-without-fix/SKILL.md @@ -1,6 +1,10 @@ --- name: verify-tests-fail-without-fix description: Verifies UI tests catch the bug. Supports two modes - verify failure only (test creation) or full verification (test + fix validation). +metadata: + author: dotnet-maui + version: "1.0" +compatibility: Requires git, PowerShell, and .NET SDK for building and running tests. --- # Verify Tests Fail Without Fix diff --git a/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1 b/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1 index 8250d5314b86..ff3fd7ff7617 100644 --- a/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1 +++ b/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1 @@ -83,129 +83,13 @@ param( $ErrorActionPreference = "Stop" $RepoRoot = git rev-parse --show-toplevel -# Test path patterns to exclude when auto-detecting fix files -$TestPathPatterns = @( - "*/tests/*", - "*/test/*", - "*.Tests/*", - "*.UnitTests/*", - "*TestCases*", - "*snapshots*", - "*.png", - "*.jpg", - ".github/*", - "*.md", - "pr-*-review.md" -) - -# Function to check if a file should be excluded from fix files -function Test-IsTestFile { - param([string]$FilePath) - - foreach ($pattern in $TestPathPatterns) { - if ($FilePath -like $pattern) { - return $true - } - } - return $false -} - # ============================================================ -# Find the merge-base commit (where current branch diverged from base) -# This is more robust than tracking branch names/refs -# For fork workflows: fetches directly from the PR's target repo URL -# so it works even if the fork's main branch is out of sync +# Import shared baseline script for merge-base and file detection # ============================================================ -function Find-MergeBase { - param([string]$ExplicitBaseBranch) - - # 1. If explicit base branch provided, use it directly - if ($ExplicitBaseBranch) { - # Try with origin/ prefix first, then without - foreach ($ref in @("origin/$ExplicitBaseBranch", $ExplicitBaseBranch)) { - $mergeBase = git merge-base HEAD $ref 2>$null - if ($mergeBase) { - return @{ MergeBase = $mergeBase; BaseBranch = $ExplicitBaseBranch; Source = "explicit" } - } - } - } - - # 2. Try to get PR metadata including the TARGET repository - # This is critical for fork workflows where origin points to the fork, - # not the upstream repo. We fetch directly from the target repo URL. - # The PR URL contains the target repo: https://github.com/OWNER/REPO/pull/123 - $prJson = gh pr view --json baseRefName,url 2>$null - if ($prJson) { - $prInfo = $prJson | ConvertFrom-Json - $prBaseBranch = $prInfo.baseRefName - $prUrl = $prInfo.url - - # Parse owner/repo from PR URL: https://github.com/OWNER/REPO/pull/123 - $targetOwner = $null - $targetRepo = $null - if ($prUrl -match "github\.com/([^/]+)/([^/]+)/pull/") { - $targetOwner = $matches[1] - $targetRepo = $matches[2] - } - - if ($prBaseBranch -and $targetOwner -and $targetRepo) { - # Construct the target repo URL and fetch directly from it - # This works even if the developer hasn't set up an 'upstream' remote - # and even if their fork's main is completely out of sync - $targetUrl = "https://github.com/$targetOwner/$targetRepo.git" - Write-Host "ℹ️ PR targets $targetOwner/$targetRepo - fetching $prBaseBranch from upstream..." -ForegroundColor Cyan - git fetch $targetUrl $prBaseBranch 2>$null - - if ($LASTEXITCODE -eq 0) { - # FETCH_HEAD now points to the target repo's base branch - $mergeBase = git merge-base HEAD FETCH_HEAD 2>$null - if ($mergeBase) { - return @{ MergeBase = $mergeBase; BaseBranch = $prBaseBranch; Source = "pr-target-repo"; TargetRepo = "$targetOwner/$targetRepo" } - } - } - } - - # Fallback: try fetching from origin (works if origin IS the target repo) - if ($prBaseBranch) { - git fetch origin $prBaseBranch 2>$null - foreach ($ref in @("origin/$prBaseBranch", $prBaseBranch)) { - $mergeBase = git merge-base HEAD $ref 2>$null - if ($mergeBase) { - return @{ MergeBase = $mergeBase; BaseBranch = $prBaseBranch; Source = "pr-metadata" } - } - } - } - } - - # 3. Fallback: Find closest merge-base among common base branch patterns - # The "correct" base is the one with fewest commits between merge-base and HEAD - Write-Host "ℹ️ No PR detected, scanning remote branches for closest base..." -ForegroundColor Cyan - - # Fetch all remote refs to ensure we have latest - git fetch origin 2>$null +$BaselineScript = Join-Path $RepoRoot ".github/scripts/EstablishBrokenBaseline.ps1" - # Get remote branches matching common base branch patterns - $remoteBranches = git branch -r --format='%(refname:short)' 2>$null | Where-Object { - $_ -match '^origin/(main|master|net\d+\.\d+|release/.*)$' - } - - $bestMatch = $null - $shortestDistance = [int]::MaxValue - - foreach ($branch in $remoteBranches) { - $mergeBase = git merge-base HEAD $branch 2>$null - if ($mergeBase) { - $distance = [int](git rev-list --count "$mergeBase..HEAD" 2>$null) - if ($distance -lt $shortestDistance) { - $shortestDistance = $distance - $branchName = $branch -replace '^origin/', '' - $bestMatch = @{ MergeBase = $mergeBase; BaseBranch = $branchName; Source = "closest-merge-base"; Distance = $distance } - } - } - } - - return $bestMatch -} +# Import Test-IsTestFile and Find-MergeBase from shared script +. $BaselineScript # ============================================================ # Auto-detect test filter from changed files diff --git a/.github/skills/write-tests/SKILL.md b/.github/skills/write-tests/SKILL.md index 0c66536a66e8..0149c1b82317 100644 --- a/.github/skills/write-tests/SKILL.md +++ b/.github/skills/write-tests/SKILL.md @@ -1,6 +1,10 @@ --- name: write-tests description: Creates UI tests for a GitHub issue and verifies they reproduce the bug. Iterates until tests actually fail (proving they catch the issue). Use when PR lacks tests or tests need to be created for an issue. +metadata: + author: dotnet-maui + version: "1.0" +compatibility: Requires git, PowerShell, .NET SDK, and Appium for UI test execution. --- # Write Tests Skill diff --git a/.gitignore b/.gitignore index a3dd5e8ae753..598def77e641 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,7 @@ templatesTest/ *.sln.docstates /src/Controls/tests/TestCases.HostApp/MauiProgram.user.cs CustomAgentLogsTmp/ +.github/.baseline-state.json # User-specific files (MonoDevelop/Xamarin Studio) *.userprefs