Perform consistent diff-informed alert filtering in the action - #2765
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
Looks good, a couple of suggestions:
Sorry, something went wrong.
| ((range.startLine <= locationStartLine && | ||
| range.endLine >= locationStartLine) || |
There was a problem hiding this comment.
What about if the location's start line isn't within the range but its end line is? If this is intentional, it might be worth adding a comment to make explicit that this case is filtered.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for the suggestion! I added comments to document the intended behavior of the filtering, by way of referencing the restrictAlertsTo extensible predicate in QL.
Sorry, something went wrong.
| } | ||
| const jsonContents = fs.readFileSync(jsonFilePath, "utf8"); | ||
| logger.debug( | ||
| `Read pr-diff-range JSON file from ${jsonFilePath}:\n${jsonContents}`, |
There was a problem hiding this comment.
It would be more robust to log only the first n entries, in case we have a very large PR.
Sorry, something went wrong.
There was a problem hiding this comment.
I will keep the logging as-is for now. Since the JSON file contains only the line ranges, hopefully it will still be manageable even for a very large PR.
Sorry, something went wrong.
| const jsonFilePath = getDiffRangesJsonFilePath(); | ||
| fs.writeFileSync(jsonFilePath, jsonContents); | ||
| logger.debug( | ||
| `Wrote pr-diff-range JSON file to ${jsonFilePath}:\n${jsonContents}`, |
There was a problem hiding this comment.
Same here
Sorry, something went wrong.
|
|
||
| function filterAlertsByDiffRange(logger: Logger, sarif: SarifFile): SarifFile { | ||
| const diffRanges = readDiffRangesJsonFile(logger); | ||
| if (!diffRanges) { |
There was a problem hiding this comment.
An empty array is truthy in ECMAScript. In the case that the PR has no changes, it might be better to test diffRanges?.length instead.
| if (!diffRanges) { | |
| if (!diffRanges?.length) { |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for the suggestion! Applied.
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
Thanks for the additional clarifications, LGTM
Sorry, something went wrong.
This PR updates the SARIF upload code path to perform consistent diff-informed alert filtering. With this change, a PR analysis with diff-informed analysis enabled will return only alerts that are within the diff range, regardless of whether the QL queries have been adapted to be diff-informed.