Remove support for combining SARIF runs with non-unique categories - #2959
Remove support for combining SARIF runs with non-unique categories#2959koesie10 merged 1 commit into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a feature-flagged error when attempting to combine multiple SARIF runs that share the same category, and updates related merge logic and tests.
- Add throwIfCombineSarifFilesDisabled & shouldDisableCombineSarifFiles to enforce non-unique-category checks behind the DisableCombineSarifFiles flag.
- Extend combineSarifFilesUsingCLI to call the new check and update deprecation messaging.
- Register and configure DisableCombineSarifFiles in the feature flags and expand test coverage accordingly.
Reviewed Changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/upload-lib.ts | Added new helper functions for feature-flagged enforcement and integrated them into merge flow |
| src/upload-lib.test.ts | Added comprehensive AVA tests for the new enforcement logic across GitHub variants |
| src/feature-flags.ts | Defined the DisableCombineSarifFiles feature flag and its environment default |
| lib/* | Generated JavaScript reflecting the above TypeScript changes (mirrors TS sources) |
src/upload-lib.ts:182
- You're using semver.lt here but there's no import for semver. Please add import * as semver from "semver"; at the top of the file.
) {
Sorry, something went wrong.
mbg
left a comment
There was a problem hiding this comment.
Thanks for taking care of this and including a bunch of tests! This generally looks good to me.
Sorry, something went wrong.
| await throwIfCombineSarifFilesDisabled( | ||
| sarifObjects, | ||
| features, | ||
| gitHubVersion, | ||
| ); |
There was a problem hiding this comment.
I assume the deprecation warning a few lines further down is not necessary if the conditions for this are true. Should it be removed now or later?
Sorry, something went wrong.
There was a problem hiding this comment.
That's true, but we can only remove the deprecation warning when the action drops support for GHES 3.17. In all GHES versions between 3.14 (including) and 3.18 (excluding), the action should still show the deprecation warning and should never give this error. Until support for GHES 3.17 is dropped, customers can still sync their action from dotcom to their GHES instance and expect it to work without breaking changes.
Sorry, something went wrong.
There was a problem hiding this comment.
Makes sense -- probably also worth tracking then
Sorry, something went wrong.
This removes support for combining SARIF runs with non-unique categories, and will instead throw a configuration error. This behavior is only enabled behind a feature flag.
This is intentionally not backwards compatible when the feature flag is enabled, as this was announced over a year ago and a deprecation warning is shown for every upload that would run into this error.
Merge / deployment checklist