[release/v7.6] Fix merge conflict checker for empty file lists and filter *.cs files #26556
Conversation
…PowerShell#26365) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: TravisEz13 <10873629+TravisEz13@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR backports infrastructure improvements from the main branch to the release/v7.6 branch, adding a new merge conflict marker detection system to prevent accidentally merging files with Git conflict markers. The implementation includes a comprehensive PowerShell function in the CI module, test coverage, and GitHub Actions integration.
Key Changes
- Adds
Test-MergeConflictMarkerfunction to detect Git conflict markers in changed files with support for empty file lists and *.cs file filtering - Creates new GitHub Actions composite action for automated merge conflict checking in pull requests
- Implements comprehensive Pester test coverage for the new functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/ci.psm1 | Adds 222-line Test-MergeConflictMarker function with empty file handling, *.cs filtering, and GitHub Actions output integration |
| test/infrastructure/ciModule.Tests.ps1 | New test file with 246 lines covering merge conflict detection scenarios, edge cases, and Install-CIPester function validation |
| .github/actions/infrastructure/merge-conflict-checker/action.yml | Composite action definition that integrates changed file detection with the merge conflict checker |
| .github/actions/infrastructure/merge-conflict-checker/README.md | Documentation explaining usage, behavior, and integration of the merge conflict checker action |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - src/example.cs | ||
| Markers found: <<<<<<<, =======, >>>>>>> | ||
| - README.md | ||
| Markers found: <<<<<<<, =======, >>>>>>> |
There was a problem hiding this comment.
The example output shows src/example.cs with conflict markers, but this contradicts the documented behavior that *.cs files are filtered out. Since C# files are excluded from merge conflict checking (as stated in line 59), they should not appear in the example output.
Update the example to show only non-cs files, for instance:
❌ Merge conflict markers detected in the following files:
- README.md
Markers found: <<<<<<<, =======, >>>>>>>
- src/example.txt
Markers found: <<<<<<<, =======, >>>>>>>
| - src/example.cs | |
| Markers found: <<<<<<<, =======, >>>>>>> | |
| - README.md | |
| Markers found: <<<<<<<, =======, >>>>>>> | |
| - README.md | |
| Markers found: <<<<<<<, =======, >>>>>>> | |
| - src/example.txt | |
| Markers found: <<<<<<<, =======, >>>>>>> |
| - **File Handling**: | ||
| - Checks only files that were added, modified, or renamed | ||
| - Skips deleted files | ||
| - **Filters out `*.cs` files** (C# files are excluded from merge conflict checking) |
There was a problem hiding this comment.
The README doesn't explain why *.cs files are excluded from merge conflict checking. Based on the PR description mentioning "exclude *.cs files from merge conflict checking to avoid false positives during C# builds", this rationale should be documented.
Suggestion:
- **Filters out `*.cs` files** (C# files are excluded from merge conflict checking to avoid false positives from C# build-generated files)This helps users understand the reasoning behind this design decision.
| - **Filters out `*.cs` files** (C# files are excluded from merge conflict checking) | |
| - **Filters out `*.cs` files** (C# files are excluded from merge conflict checking to avoid false positives from C# build-generated files, which may contain patterns resembling merge conflict markers) |
| Context "When handling special file scenarios" { | ||
| It "Should skip non-existent files" { | ||
| Test-MergeConflictMarker -File @("nonexistent.txt") -WorkspacePath $script:testWorkspace -OutputPath $script:testOutputPath -SummaryPath $script:testSummaryPath | ||
|
|
||
| $outputs = Get-Content $script:testOutputPath | ||
| $outputs | Should -Contain "files-checked=0" | ||
| } | ||
|
|
||
| It "Should handle absolute paths" { | ||
| $testFile = Join-Path $script:testWorkspace "absolute.txt" | ||
| "Clean content" | Out-File $testFile -Encoding utf8 | ||
|
|
||
| Test-MergeConflictMarker -File @($testFile) -WorkspacePath $script:testWorkspace -OutputPath $script:testOutputPath -SummaryPath $script:testSummaryPath | ||
|
|
||
| $outputs = Get-Content $script:testOutputPath | ||
| $outputs | Should -Contain "conflicts-found=0" | ||
| } | ||
|
|
||
| It "Should handle mixed relative and absolute paths" { | ||
| $testFile1 = Join-Path $script:testWorkspace "relative.txt" | ||
| "Clean" | Out-File $testFile1 -Encoding utf8 | ||
|
|
||
| $testFile2 = Join-Path $script:testWorkspace "absolute.txt" | ||
| "Clean" | Out-File $testFile2 -Encoding utf8 | ||
|
|
||
| Test-MergeConflictMarker -File @("relative.txt", $testFile2) -WorkspacePath $script:testWorkspace -OutputPath $script:testOutputPath -SummaryPath $script:testSummaryPath | ||
|
|
||
| $outputs = Get-Content $script:testOutputPath | ||
| $outputs | Should -Contain "files-checked=2" | ||
| $outputs | Should -Contain "conflicts-found=0" | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the *.cs file filtering feature. The PR description and implementation include logic to filter out *.cs files from merge conflict checking (lines 1057-1069 in ci.psm1), but there are no tests validating this behavior.
Suggested tests to add:
- Test that *.cs files are filtered out when provided
- Test that the filtered count is correctly reported
- Test that when all files are *.cs, the function handles it gracefully (already implemented in code but not tested)
- Test mixed scenarios with both *.cs and non-cs files
Example test:
It "Should filter out *.cs files from checking" {
$testCsFile = Join-Path $script:testWorkspace "file.cs"
$testTxtFile = Join-Path $script:testWorkspace "file.txt"
"Clean content" | Out-File $testCsFile -Encoding utf8
"Clean content" | Out-File $testTxtFile -Encoding utf8
Test-MergeConflictMarker -File @("file.cs", "file.txt") -WorkspacePath $script:testWorkspace -OutputPath $script:testOutputPath -SummaryPath $script:testSummaryPath
$outputs = Get-Content $script:testOutputPath
$outputs | Should -Contain "files-checked=1" # Only txt file checked
}
Backport of #26365 to release/v7.6
Triggered by @adityapatwardhan on behalf of @app/copilot-swe-agent
Original CL Label: CL-Test
/cc @PowerShell/powershell-maintainers
Impact
REQUIRED: Choose either Tooling Impact or Customer Impact (or both). At least one checkbox must be selected.
Tooling Impact
CI infrastructure fix to prevent CI failures when PRs only delete files, and exclude *.cs files from merge conflict checking to avoid false positives during C# builds
Customer Impact
Regression
REQUIRED: Check exactly one box.
This is not a regression.
Testing
Original PR included comprehensive test coverage for new Test-MergeConflictMarker function. Backport tested by successful cherry-pick with conflict resolution.
Risk
REQUIRED: Check exactly one box.
Test-only infrastructure change with comprehensive test coverage. Creates new GitHub Actions infrastructure and CI function but doesn't modify core PowerShell functionality.
Merge Conflicts
Resolved conflicts in 4 files - created new infrastructure files that don't exist in v7.6 branch and merged new function into existing ci.psm1 module.