-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add gh-aw workflow for automated prior fix regression detection #34768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| --- | ||
| description: "How to detect and prevent regression bugs caused by PRs that inadvertently remove lines added by prior bug fix PRs. Apply during code reviews of any PR that removes lines from implementation files." | ||
| applyTo: "**/*.cs,**/*.xaml,**/*.swift,**/*.java,**/*.kt" | ||
| --- | ||
|
|
||
| # Regression Detection: Reverted Fix Lines | ||
|
|
||
| ## The Problem | ||
|
|
||
| A critical class of regression bugs occurs when a PR fixes issue A but silently removes a line that was added to fix issue B. The reviewer approves the PR thinking it only fixes A, unaware that B has been reintroduced. | ||
|
|
||
| ### Real Example from dotnet/maui | ||
|
|
||
| **PR #33908** fixed issue #33604 (CollectionView not respecting SafeAreaEdges). To fix this, the PR removed `|| parent is IMauiRecyclerView` from `FindListenerForView()` in `MauiWindowInsetListener.cs`. | ||
|
|
||
| **What the reviewer didn't know:** That exact line was added by **PR #32278** to fix **issue #32436** (increasing gap at bottom while scrolling in RecyclerView). Removing it caused bug #32436 to reappear as **regression #34634**, shipping in 10.0.60. | ||
|
|
||
| **Why it was missed:** The removed line looked intentional β PR #33908 explained that the `IMauiRecyclerView` check was preventing CollectionView items from receiving window inset listeners. The explanation was correct for the NEW issue. But neither the author nor the reviewer knew about the OLD issue the line was protecting against. | ||
|
|
||
| ## Detection Method | ||
|
|
||
| When reviewing any PR that **deletes lines** from implementation files, run the prior fix regression check: | ||
|
|
||
| ### Step 1: Identify Significant Deleted Lines | ||
|
|
||
| Look at the PR diff for deleted lines (`-` prefix) in non-test `.cs`, `.xaml`, `.swift`, `.java`, or `.kt` files. | ||
|
|
||
| Skip trivial deletions: | ||
| - Blank lines | ||
| - Pure comments (`//`, `/* */`, `/** */`) | ||
| - Opening/closing braces alone (`{`, `}`) | ||
| - `using` statements and `namespace` declarations | ||
|
|
||
| **Pay extra attention to:** | ||
| - Condition guards: `if (x is SomeType)`, `|| condition`, `&& condition` | ||
| - Type checks: `parent is IMauiRecyclerView`, `view is IScrollView` | ||
| - Null checks added after null reference exceptions | ||
| - Try/catch blocks for specific exception types | ||
| - Special-case handling for edge conditions | ||
|
|
||
| ### Step 2: Check Git Blame on Deleted Lines | ||
|
|
||
| ```bash | ||
| # Check who added a specific line and why | ||
| git blame main -- src/path/to/file.cs | grep -F "deleted line content" | ||
|
|
||
| # Get the full commit message for that hash | ||
| git log --format="%s%n%b" -1 <commit-hash> | ||
|
|
||
| # Or use the script: | ||
| pwsh .github/skills/pr-finalize/scripts/Detect-Regressions.ps1 -PRNumber XXXXX | ||
| ``` | ||
|
|
||
| ### Step 3: Check for Issue References | ||
|
|
||
| In the commit message, look for: | ||
| - `fixes #XXXX`, `closes #XXXX`, `resolves #XXXX` | ||
| - `issue #XXXX`, `re: #XXXX` | ||
| - PR references like `(#XXXX)` in the subject line | ||
|
|
||
| ### Step 4: Flag if Origin is a Bug Fix | ||
|
|
||
| **Flag as π΄ Critical** if: | ||
| - The deleted line's originating commit references an issue number, AND | ||
| - The PR being reviewed does NOT mention that the original issue is still addressed by other means | ||
|
|
||
| ## What to Output | ||
|
|
||
| ### When a Regression is Detected | ||
|
|
||
| ````markdown | ||
| ### π΄ Prior Fix Regression Detected | ||
|
|
||
| **File:** `src/Core/src/Platform/Android/MauiWindowInsetListener.cs` | ||
|
|
||
| **Removed line:** | ||
| ```diff | ||
| - (parent is AppBarLayout || parent is MauiScrollView || parent is IMauiRecyclerView) | ||
| + (parent is AppBarLayout || parent is MauiScrollView) | ||
| ``` | ||
|
|
||
| **Origin:** Added in commit `0b30658` (PR #32278): | ||
| > "[Android] Refactor WindowInsetListener to per-view registry with MauiWindowInsetListener" | ||
|
|
||
| **References issue:** #32436 (increasing gap at bottom while scrolling) | ||
|
|
||
| **Risk:** Removing this guard will cause RecyclerView/CollectionView scroll to reintroduce the bottom gap (issue #32436). | ||
|
|
||
| **Action required:** PR author must confirm this removal is intentional AND explain how issue #32436 is prevented by other means in this PR. | ||
| ```` | ||
|
|
||
| ### When Check Passes | ||
|
|
||
| ````markdown | ||
| ### β Prior Fix Regression Check: PASSED | ||
|
|
||
| No deleted lines were identified as reversions of prior bug fixes. | ||
| ```` | ||
|
|
||
| ## Guidance for PR Authors | ||
|
|
||
| When your PR removes lines, explicitly document WHY in the PR description: | ||
|
|
||
| ```markdown | ||
| ### Removed Lines (with justification) | ||
|
|
||
| - Removed `|| parent is IMauiRecyclerView` from `FindListenerForView()` | ||
| - **Original purpose:** Added in PR #32278 to fix issue #32436 (bottom gap in RecyclerView) | ||
| - **Why safe to remove now:** This PR adds a more targeted fix in `CollectionViewHandler.cs` that | ||
| handles the RecyclerView inset case directly, making the blanket exclusion in `FindListenerForView()` | ||
| unnecessary. Verified that issue #32436 does not reproduce with this PR. | ||
| ``` | ||
|
|
||
| ## High-Risk Patterns in dotnet/maui | ||
|
|
||
| These code patterns in MAUI are frequently the targets of "reverted fix" regressions: | ||
|
|
||
| ### Android Window Insets (`MauiWindowInsetListener.cs`) | ||
|
|
||
| Type guards in `FindListenerForView()` that exclude specific view types from inset handling: | ||
| ```csharp | ||
| // Each of these guards was added to fix a specific bug β do NOT remove without understanding why | ||
| if (view is not MaterialToolbar && | ||
| (parent is AppBarLayout || parent is MauiScrollView || parent is IMauiRecyclerView)) | ||
| { | ||
| return null; | ||
| } | ||
| ``` | ||
|
|
||
| ### iOS Safe Area (`MauiView.cs`, `SafeAreaExtensions.cs`) | ||
|
|
||
| Checks that prevent double-application of safe area padding: | ||
| ```csharp | ||
| // Guards preventing double-padding β each added to fix a specific issue | ||
| if (IsParentHandlingSafeArea(view, edge)) return; | ||
| if (EqualsAtPixelLevel(current, insets)) return; | ||
| ``` | ||
|
|
||
| ### Android RecyclerView Padding (`CollectionViewHandler.Android.cs`) | ||
|
|
||
| Padding resets that prevent cumulative padding bugs: | ||
| ```csharp | ||
| // Reset before applying β added to fix issue #XXXXX where padding accumulated on scroll | ||
| view.SetPadding(0, 0, 0, 0); | ||
| ``` | ||
|
|
||
| ## Integration with Skills | ||
|
|
||
| This check is built into: | ||
| - **`pr-finalize/SKILL.md`** β Phase 2, Step 1 (runs before all other code review) | ||
| - **`pr-finalize/scripts/Detect-Regressions.ps1`** β Automated detection script | ||
|
|
||
| When the `pr-finalize` skill is invoked, the Prior Fix Regression Check runs automatically as the first step of Phase 2. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -311,9 +311,94 @@ Fixed the issue mentioned in #30897 | |
|
|
||
| After verifying title/description, perform a **code review** to catch best practice violations and potential issues before merge. | ||
|
|
||
| ### Review Focus Areas | ||
| ### π¨ STEP 1 (MANDATORY): Prior Fix Regression Check | ||
|
|
||
| When reviewing code changes, focus on: | ||
| **Run this check FIRST, before any other code review.** This is the highest-priority check because it catches the most dangerous class of bugs: silently reverting prior fixes. | ||
|
|
||
| #### Background | ||
|
|
||
| PRs that fix one bug sometimes inadvertently remove lines that were added to fix a *different* bug. These "reverted fix" regressions are dangerous because: | ||
| - The PR author doesn't know the history of the removed line | ||
| - The change looks intentional β the PR description only mentions the NEW issue being fixed | ||
| - CI tests pass because the regressed bug often has no automated test | ||
| - The regression ships silently alongside the fix | ||
|
|
||
| **Real example:** PR #33908 ([Android/iOS] Fix CollectionView not respecting SafeAreaEdges) removed `|| parent is IMauiRecyclerView` from `FindListenerForView()`. This line was added by PR #32278 specifically to fix issue #32436 (gap at bottom while scrolling in RecyclerView). Removing it caused bug #32436 to reappear as regression #34634. | ||
|
|
||
| #### How to Run the Check | ||
|
|
||
| ```bash | ||
| # Step 1: Get all deleted lines from non-test implementation files | ||
| gh pr diff XXXXX | grep "^-" | grep -v "^---" | grep -v "^-.*\(test\|Test\|spec\|Spec\)" > /tmp/deleted_lines.txt | ||
|
|
||
| # Step 2: For each deleted non-trivial line (not blank, not comment-only, not brace-only), | ||
| # find where it came from using git blame on the base branch | ||
| # The script automates this: | ||
| pwsh .github/skills/pr-finalize/scripts/Detect-Regressions.ps1 -PRNumber XXXXX | ||
|
Comment on lines
+330
to
+337
|
||
| ``` | ||
|
|
||
| #### Manual Check (if script unavailable) | ||
|
|
||
| For each **implementation file** (not test files) in the diff: | ||
|
|
||
| ```bash | ||
| # See what was deleted from a specific file | ||
| gh pr diff XXXXX -- src/path/to/file.cs | grep "^-" | grep -v "^---" | ||
|
|
||
| # For suspicious deleted lines, check git blame to see what commit added them | ||
| # First, find the line number in the base branch: | ||
| git blame origin/main -- src/path/to/file.cs | grep -F "deleted line content" | ||
| ``` | ||
|
|
||
| #### What to Flag as π΄ Critical | ||
|
|
||
| Flag as **π΄ Critical β Prior Fix Regression** when ALL of the following are true: | ||
|
|
||
| 1. A line was **deleted** from an implementation file (not a test file) | ||
| 2. Git blame shows that line was added in a **specific prior commit** (not just "always been there") | ||
| 3. That prior commit's message references an **issue number** (e.g., "fixes #32436", "closes #1234") OR the commit message explicitly describes a bug fix | ||
| 4. The PR being reviewed does **not** explain why removing that line is intentional/safe | ||
|
|
||
| **Special attention to:** | ||
| - Condition guards: `if (x is SomeType)`, `|| condition`, `&& condition` β these are common guard patterns added to prevent bugs | ||
| - Null checks added after null reference exceptions | ||
| - Type checks like `parent is IMauiRecyclerView` that prevent specific views from receiving certain handling | ||
| - Exception handlers added for specific error cases | ||
|
|
||
| #### Output for Regression Check | ||
|
|
||
| If regressions found: | ||
| ```` | ||
| ### π΄ Prior Fix Regression Detected | ||
|
|
||
| **File:** `src/Core/src/Platform/Android/MauiWindowInsetListener.cs` | ||
|
|
||
| **Removed line:** | ||
| ```diff | ||
| - (parent is AppBarLayout || parent is MauiScrollView || parent is IMauiRecyclerView) | ||
| + (parent is AppBarLayout || parent is MauiScrollView) | ||
| ``` | ||
|
|
||
| **Origin:** This line was added in commit `0b30658` (PR #32278) which states: | ||
| > "Fix issue #32436 β increasing gap at bottom while scrolling" | ||
|
|
||
| **Risk:** Removing this guard will cause RecyclerView/CollectionView scrolling to re-introduce the bottom gap regression (issue #32436). | ||
|
|
||
| **Required action before merge:** Author must confirm this removal is intentional AND explain how the original bug #32436 is now prevented by other means. | ||
| ```` | ||
|
|
||
| If no regressions found: | ||
| ```` | ||
| ### β Prior Fix Regression Check: PASSED | ||
|
|
||
| No deleted lines were identified as reversions of prior bug fixes. | ||
| ```` | ||
|
|
||
| --- | ||
|
|
||
| ### STEP 2: General Code Review | ||
|
|
||
| After completing the regression check, review code changes for: | ||
|
|
||
| 1. **Code quality and maintainability** - Clean code, good naming, appropriate abstractions | ||
| 2. **Error handling and edge cases** - Null checks, exception handling, boundary conditions | ||
|
|
@@ -336,6 +421,9 @@ gh pr diff XXXXX -- path/to/file.cs | |
| ```markdown | ||
| ## Code Review Findings | ||
|
|
||
| ### π΄ Prior Fix Regression Check | ||
| [Result from Step 1 above β always present] | ||
|
|
||
| ### π΄ Critical Issues | ||
|
|
||
| **[Issue Title]** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guidance uses
git blame main -- ..., but in many local clones/CI environments the base branch ref isorigin/main(andmainmight not exist as a local branch). Consider usinggit blame origin/main -- ...(or explicitly noting βyour base branchβ) to avoid copy/paste failures.