Perform consistent diff-informed alert filtering in the action#2765
Perform consistent diff-informed alert filtering in the action#2765
Conversation
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
cklin
force-pushed
the
cklin/alert-diff-filtering
branch
from
7004011 to
7e9a076
Compare
henrymercer
left a comment
henrymercer
left a comment
There was a problem hiding this comment.
Looks good, a couple of suggestions:
| ((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.
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.
| } | ||
| 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.
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.
| const jsonFilePath = getDiffRangesJsonFilePath(); | ||
| fs.writeFileSync(jsonFilePath, jsonContents); | ||
| logger.debug( | ||
| `Wrote pr-diff-range JSON file to ${jsonFilePath}:\n${jsonContents}`, |
src/upload-lib.ts
Outdated
|
|
||
| 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) { |
There was a problem hiding this comment.
Thanks for the suggestion! Applied.
cklin
force-pushed
the
cklin/alert-diff-filtering
branch
from
7e9a076 to
fb0b66d
Compare
cklin
force-pushed
the
cklin/alert-diff-filtering
branch
from
fb0b66d to
f85d8b5
Compare
henrymercer
left a comment
henrymercer
left a comment
There was a problem hiding this comment.
Thanks for the additional clarifications, LGTM
2 participants
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.