-
Notifications
You must be signed in to change notification settings - Fork 402
Make the overlay changed files always include the diff #3192
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?
Conversation
We don't use them yet and will re-save them during analysis.
ab7a163
to
f030ca3
Compare
44ed913
to
5a8e345
Compare
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.
Pull Request Overview
This PR integrates diff-informed analysis into overlay mode by computing PR diff ranges during init and including changed files from the diff in overlay analysis. This ensures that overlay analysis always processes files with at least one edited range, improving the coverage of diff-informed analysis.
Key changes:
- Moves diff range computation from analyze to init step to enable early persistence and reuse
- Augments overlay changed files with files from PR diff ranges
- Refactors diff-informed analysis logic to use precomputed ranges instead of computing them during analyze
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/overlay-database-utils.ts | Enhanced to accept PR diff changed files and include them in overlay analysis |
src/init-action.ts | Added early computation and persistence of diff ranges with new helper function |
src/diff-informed-analysis-utils.ts | Moved diff computation logic from analyze.ts and added export functions |
src/analyze.ts | Refactored to use precomputed diff ranges instead of computing them inline |
src/codeql.ts | Updated interface to accept PR diff changed files parameter |
Comments suppressed due to low confidence (1)
src/diff-informed-analysis-utils.ts:1
- The replaceAll method may not be available in older Node.js versions. Consider using replace with a global regex for better compatibility: .replace(new RegExp('\\' + path.sep, 'g'), '/')
import * as fs from "fs";
} | ||
if (extraAddedCount > 0) { | ||
logger.debug( | ||
`Added ${extraAddedCount} file(s) from PR diff ranges into overlay: ${changedFiles.slice(-extraAddedCount).join(", ")}`, |
Copilot
AI
Oct 10, 2025
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.
The slice operation on changedFiles array could be inefficient for large arrays. Consider tracking the added files separately to avoid the slice operation on potentially large arrays.
Copilot uses AI. Check for mistakes.
return undefined; | ||
} | ||
writeDiffRangesJsonFile(logger, ranges); | ||
const distinctFiles = new Set(ranges.map((r) => r.path)); |
Copilot
AI
Oct 10, 2025
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.
Creating a Set from mapped array could be optimized by using a for loop to avoid creating an intermediate array, especially for large numbers of ranges.
const distinctFiles = new Set(ranges.map((r) => r.path)); | |
const distinctFiles = new Set<string>(); | |
for (const r of ranges) { | |
distinctFiles.add(r.path); | |
} |
Copilot uses AI. Check for mistakes.
This includes the PR diff in the overlay changes so that any "overlay informed analysis" runs in a detailed way on those files..
This requires writing
pr-diff-range.json
ininit
and reading insideanalyse
.A minor change is that for an empty PR diff, a dummy entry is not written to
pr-diff-range.json
.analyse
still adds it to the generated pack, but it isn't used by `upload``.I am assuming that mixing and matching init and analyse from different action versions is not supported.
Risk assessment
Which use cases does this change impact?
This affects all cases where diff-informed may apply:
analysis-kinds: code-scanning
).How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Merge / deployment checklist