-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Add GitHub Action to detect overlapping PRs #76820
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: trunk
Are you sure you want to change the base?
Changes from 6 commits
2361f5b
8589fe6
526a7c0
bf13768
e812dc0
8683ae0
36ff4bb
eb58731
60643a3
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,242 @@ | ||||||
| name: Overlapping PRs | ||||||
|
|
||||||
| on: | ||||||
| pull_request: | ||||||
| types: | ||||||
| - opened | ||||||
| - synchronize | ||||||
|
|
||||||
| # Cancels all previous workflow runs for pull requests that have not completed. | ||||||
| concurrency: | ||||||
| group: ${{ github.workflow }}-${{ github.head_ref }} | ||||||
| cancel-in-progress: true | ||||||
|
|
||||||
| # Disable permissions for all available scopes by default. | ||||||
| # Any needed permissions should be configured at the job level. | ||||||
| permissions: {} | ||||||
|
|
||||||
| jobs: | ||||||
| overlapping-prs: | ||||||
| name: Find overlapping PRs | ||||||
| runs-on: ubuntu-24.04 | ||||||
| permissions: | ||||||
| pull-requests: write | ||||||
| contents: read | ||||||
| timeout-minutes: 10 | ||||||
| steps: | ||||||
| - name: Find PRs touching the same lines | ||||||
| uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 | ||||||
| with: | ||||||
| retries: 2 | ||||||
| retry-exempt-status-codes: 418 | ||||||
| script: | | ||||||
| const prNumber = context.payload.pull_request.number; | ||||||
|
|
||||||
| // Files to ignore — these always overlap and conflicts are trivial. | ||||||
| const IGNORED_PATTERNS = [ | ||||||
| 'package-lock.json', | ||||||
| 'composer.lock', | ||||||
| '**/CHANGELOG.md', | ||||||
| ]; | ||||||
|
|
||||||
| function isIgnored( filename ) { | ||||||
| return IGNORED_PATTERNS.some( ( pattern ) => { | ||||||
| if ( pattern.startsWith( '**/' ) ) { | ||||||
| return filename.endsWith( pattern.slice( 2 ) ); | ||||||
| } | ||||||
| return filename === pattern; | ||||||
| } ); | ||||||
| } | ||||||
|
Comment on lines
+37
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works well for the initial implementation, but it may not be sufficient to handle more complex patterns if/when the list grows. We should consider using a more complete tool (maybe the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I've added a comment documenting exactly what the pattern matcher supports (exact match and |
||||||
|
|
||||||
| // Parse hunk headers from a patch string. | ||||||
| // Returns an array of { start, end } line ranges (base side). | ||||||
| function parseHunks( patch ) { | ||||||
| if ( ! patch ) { | ||||||
| return []; | ||||||
| } | ||||||
| const hunks = []; | ||||||
| const regex = /^@@ -(\d+)(?:,(\d+))? /gm; | ||||||
| let match; | ||||||
| while ( ( match = regex.exec( patch ) ) !== null ) { | ||||||
| const start = parseInt( match[ 1 ], 10 ); | ||||||
| const count = match[ 2 ] !== undefined ? parseInt( match[ 2 ], 10 ) : 1; | ||||||
| if ( count > 0 ) { | ||||||
| hunks.push( { start, end: start + count - 1 } ); | ||||||
| } | ||||||
| } | ||||||
| return hunks; | ||||||
| } | ||||||
|
Comment on lines
+60
to
+82
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, this function can highlight is two PRs touch the same region of the original file, but it may miss instances in which two or more PRs add code at the same insertion point but at different new-side offsets. Probably fine since this is mostly a "heads-up" tool, but we should at least acknowledge the limitation |
||||||
|
|
||||||
| // Check if two sets of line ranges overlap. | ||||||
| function rangesOverlap( rangesA, rangesB ) { | ||||||
| for ( const a of rangesA ) { | ||||||
| for ( const b of rangesB ) { | ||||||
| if ( a.start <= b.end && b.start <= a.end ) { | ||||||
| return true; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| // Get files changed in this PR with their patches. | ||||||
| const changedFiles = new Map(); | ||||||
| for await ( const response of github.paginate.iterator( | ||||||
| github.rest.pulls.listFiles, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This workflow runs on every change to every PR, causing 2000+ API requests per workflow run (there are currently 2k open PRs), meaning that it can singlehandedly exceed the rate limit of 15k/hour that is shared across the repo. Claude suggests a few ideas to improve the situation:
|
||||||
| { | ||||||
| owner: context.repo.owner, | ||||||
| repo: context.repo.repo, | ||||||
| pull_number: prNumber, | ||||||
| per_page: 100, | ||||||
| } | ||||||
| ) ) { | ||||||
| for ( const file of response.data ) { | ||||||
| if ( ! isIgnored( file.filename ) ) { | ||||||
| changedFiles.set( file.filename, parseHunks( file.patch ) ); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if ( changedFiles.size === 0 ) { | ||||||
| core.info( 'No changed files found.' ); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| core.info( `This PR touches ${ changedFiles.size } file(s).` ); | ||||||
|
|
||||||
| // Get all open PRs (including drafts). | ||||||
| const openPRs = []; | ||||||
| for await ( const response of github.paginate.iterator( | ||||||
| github.rest.pulls.list, | ||||||
| { | ||||||
| owner: context.repo.owner, | ||||||
| repo: context.repo.repo, | ||||||
| state: 'open', | ||||||
| per_page: 100, | ||||||
| } | ||||||
| ) ) { | ||||||
| for ( const pr of response.data ) { | ||||||
| if ( pr.number !== prNumber ) { | ||||||
| openPRs.push( pr ); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| core.info( `Found ${ openPRs.length } other open PR(s) to check.` ); | ||||||
|
|
||||||
| // For each open PR, get its changed files and check for line overlaps. | ||||||
| const overlapping = []; | ||||||
|
|
||||||
| for ( const pr of openPRs ) { | ||||||
| const prFiles = []; | ||||||
| for await ( const response of github.paginate.iterator( | ||||||
| github.rest.pulls.listFiles, | ||||||
| { | ||||||
| owner: context.repo.owner, | ||||||
| repo: context.repo.repo, | ||||||
| pull_number: pr.number, | ||||||
| per_page: 100, | ||||||
| } | ||||||
| ) ) { | ||||||
| for ( const file of response.data ) { | ||||||
| prFiles.push( file ); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| const filesWithOverlap = []; | ||||||
| for ( const file of prFiles ) { | ||||||
| const ourHunks = changedFiles.get( file.filename ); | ||||||
| if ( ! ourHunks ) { | ||||||
| continue; | ||||||
| } | ||||||
| const theirHunks = parseHunks( file.patch ); | ||||||
| // If either side has no hunks (e.g. binary file), fall back to file-level match. | ||||||
| if ( ourHunks.length === 0 || theirHunks.length === 0 || rangesOverlap( ourHunks, theirHunks ) ) { | ||||||
| filesWithOverlap.push( file.filename ); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if ( filesWithOverlap.length > 0 ) { | ||||||
| overlapping.push( { | ||||||
| number: pr.number, | ||||||
| title: pr.title, | ||||||
| url: pr.html_url, | ||||||
| draft: pr.draft, | ||||||
| author: pr.user.login, | ||||||
| sharedFiles: filesWithOverlap, | ||||||
| } ); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Build comment body. | ||||||
| const marker = '<!-- overlapping-prs-bot -->'; | ||||||
| let body; | ||||||
|
|
||||||
| if ( overlapping.length === 0 ) { | ||||||
| core.info( 'No overlapping PRs found.' ); | ||||||
| // Remove existing comment if no overlaps remain. | ||||||
| const comments = await github.paginate( | ||||||
| github.rest.issues.listComments, | ||||||
| { | ||||||
| owner: context.repo.owner, | ||||||
| repo: context.repo.repo, | ||||||
| issue_number: prNumber, | ||||||
| per_page: 100, | ||||||
| } | ||||||
| ); | ||||||
| const existing = comments.find( ( c ) => c.body.includes( marker ) ); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the PR body is empty, this could throw. We should add a guard, something like
Suggested change
|
||||||
| if ( existing ) { | ||||||
| await github.rest.issues.deleteComment( { | ||||||
| owner: context.repo.owner, | ||||||
| repo: context.repo.repo, | ||||||
| comment_id: existing.id, | ||||||
| } ); | ||||||
| core.info( 'Deleted stale overlapping PRs comment.' ); | ||||||
| } | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| body = `${ marker }\n`; | ||||||
| body += `## Overlapping PRs\n\n`; | ||||||
| body += `The following open PRs modify the same lines of code as this PR:\n\n`; | ||||||
|
|
||||||
| for ( const pr of overlapping ) { | ||||||
| const draftLabel = pr.draft ? ' (draft)' : ''; | ||||||
| body += `### #${ pr.number }${ draftLabel } — ${ pr.title }\n`; | ||||||
| body += `by @${ pr.author }\n\n`; | ||||||
| body += `<details>\n<summary>Overlapping files (${ pr.sharedFiles.length })</summary>\n\n`; | ||||||
| for ( const file of pr.sharedFiles ) { | ||||||
| body += `- \`${ file }\`\n`; | ||||||
| } | ||||||
| body += `\n</details>\n\n`; | ||||||
| } | ||||||
|
|
||||||
| // Find existing comment or create a new one. | ||||||
| const comments = await github.paginate( | ||||||
| github.rest.issues.listComments, | ||||||
| { | ||||||
| owner: context.repo.owner, | ||||||
| repo: context.repo.repo, | ||||||
| issue_number: prNumber, | ||||||
| per_page: 100, | ||||||
| } | ||||||
| ); | ||||||
| const existing = comments.find( ( c ) => c.body.includes( marker ) ); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as previous comment |
||||||
|
|
||||||
| if ( existing ) { | ||||||
| await github.rest.issues.updateComment( { | ||||||
| owner: context.repo.owner, | ||||||
| repo: context.repo.repo, | ||||||
| comment_id: existing.id, | ||||||
| body, | ||||||
| } ); | ||||||
| core.info( `Updated existing comment with ${ overlapping.length } overlapping PR(s).` ); | ||||||
| } else { | ||||||
| await github.rest.issues.createComment( { | ||||||
| owner: context.repo.owner, | ||||||
| repo: context.repo.repo, | ||||||
| issue_number: prNumber, | ||||||
| body, | ||||||
| } ); | ||||||
| core.info( `Created comment with ${ overlapping.length } overlapping PR(s).` ); | ||||||
| } | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reminder to remove these test changes before merging 🧹 |
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.
Just confirming that
pull_request_targetshould be the right trigger here (acknowledging that @scruffian already mentinoed it, too)