Diff-informed analysis: fix empty PR handling #2818
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes how diff-informed analysis handles empty pull requests to ensure that the absence of added or modified lines does not inadvertently include all alerts.
- Added a check for empty diff ranges in both JavaScript and TypeScript implementations
- Replaces an empty diff range with a dummy range that cannot match any alert location
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/analyze.js | Adds a check for an empty diff range and sets a dummy range. |
| src/analyze.ts | Adds a similar check and dummy range for empty diff scenarios. |
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
| // interprets an empty data extension differently, as an indication that | ||
| // all alerts should be included. So we need to specifically set the diff | ||
| // range to a non-empty list that cannot match any alert location. | ||
| ranges = [{ path: "", startLine: 0, endLine: 0 }]; |
There was a problem hiding this comment.
I was wondering whether we have QL logic that looks for locations overlapping with the diff range. But I think that is not possible with this choice of range.
This looks right as long as we don't try to check the path against actual paths in the source archive.
There was a problem hiding this comment.
When start line and end line are both 0, the QL logic that matches location overlapping with the diff range is a simple join on the absolute path. Since an absolute path in a PR diff cannot be empty, such a join should very efficiently discard all valid potential alert locations.
This PR fixes how diff-informed analysis handles empty pull requests.
Merge / deployment checklist