-
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 all 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,290 @@ | ||
| name: Overlapping PRs | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: | ||
| - opened | ||
| - synchronize | ||
| - labeled | ||
|
|
||
| # 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 | ||
| if: contains(github.event.pull_request.labels.*.name, 'check-overlaps') | ||
| 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. | ||
| // Supported pattern syntax (intentionally minimal): | ||
| // - Exact match: 'package-lock.json' matches only that path. | ||
| // - '**/suffix': matches any file whose path ends with '/suffix' | ||
| // (e.g. '**/CHANGELOG.md' matches 'packages/foo/CHANGELOG.md'). | ||
| // More complex globs (e.g. '*.json', 'packages/*/src/**') are NOT | ||
| // supported. If richer patterns are needed in the future, consider | ||
| // pulling in a glob library like minimatch. | ||
| 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). | ||
| // Note: this compares base-side line ranges, so it catches edits | ||
| // to the same original lines. It may miss pure insertions at the | ||
| // same point if the base-side context ranges don't overlap (e.g. | ||
| // two PRs both inserting at line 100 but with ,0 count). This is | ||
| // acceptable since the tool is a heads-up, not a merge-conflict detector. | ||
| 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).` ); | ||
|
|
||
| // Marker used to identify the bot's comment for updates/deletion. | ||
| const marker = '<!-- overlapping-prs-bot -->'; | ||
|
|
||
| // Helper to clean up a stale bot comment when there are no overlaps. | ||
| async function deleteStaleComment() { | ||
| 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 && c.body.includes( marker ) ); | ||
| 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.' ); | ||
| } | ||
| } | ||
|
|
||
| // Pre-filter: use the Search API to find PRs that touch the same files. | ||
| // This avoids fetching patches for all 2000+ open PRs and blowing | ||
| // through the API rate limit. | ||
| const candidatePRs = new Map(); // PR number -> { number, title, ... } | ||
| const ourFilenames = [ ...changedFiles.keys() ]; | ||
|
|
||
| for ( const filename of ourFilenames ) { | ||
| // Search for open PRs (excluding ours) that touch this file. | ||
| // The search API is much cheaper than listing files for every PR. | ||
| const query = `repo:${ context.repo.owner }/${ context.repo.repo } is:pr is:open "${ filename }" -number:${ prNumber }`; | ||
| try { | ||
| const searchResults = await github.rest.search.issuesAndPullRequests( { | ||
| q: query, | ||
| per_page: 100, | ||
| } ); | ||
| for ( const item of searchResults.data.items ) { | ||
| if ( ! candidatePRs.has( item.number ) ) { | ||
| candidatePRs.set( item.number, { | ||
| number: item.number, | ||
| title: item.title, | ||
| draft: item.draft, | ||
| author: item.user.login, | ||
| html_url: item.html_url, | ||
| } ); | ||
| } | ||
| } | ||
| } catch ( e ) { | ||
| core.warning( `Search failed for file "${ filename }": ${ e.message }` ); | ||
| } | ||
| } | ||
|
|
||
| core.info( `Search found ${ candidatePRs.size } candidate PR(s) sharing filenames.` ); | ||
|
|
||
| if ( candidatePRs.size === 0 ) { | ||
| core.info( 'No candidate PRs found — skipping patch comparison.' ); | ||
| await deleteStaleComment(); | ||
| return; | ||
| } | ||
|
|
||
| // Check rate limit before the expensive part. | ||
| const rateLimit = await github.rest.rateLimit.get(); | ||
| const remaining = rateLimit.data.resources.core.remaining; | ||
| core.info( `API rate limit remaining: ${ remaining }` ); | ||
| if ( remaining < 500 ) { | ||
| core.warning( `Rate limit too low (${ remaining } remaining) — skipping patch comparison.` ); | ||
| return; | ||
| } | ||
|
|
||
| // For each candidate PR, get its changed files and check for line overlaps. | ||
| const overlapping = []; | ||
|
|
||
| for ( const [ , pr ] of candidatePRs ) { | ||
| 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.author, | ||
| sharedFiles: filesWithOverlap, | ||
| } ); | ||
| } | ||
| } | ||
|
|
||
| // Build comment body. | ||
| let body; | ||
|
|
||
| if ( overlapping.length === 0 ) { | ||
| core.info( 'No overlapping PRs found.' ); | ||
| await deleteStaleComment(); | ||
| 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 && c.body.includes( marker ) ); | ||
|
|
||
| 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)