- Notifications
You must be signed in to change notification settings - Fork428
Fixupload-sarif potentially initialising CodeQL twice#3006
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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
The PR fixes a bug where theupload-sarif action could initialize the CodeQL CLI twice when uploading multiple SARIF files. This occurred becausecombineSarifFilesUsingCLI could be called multiple times in the execution path and each call would initialize CodeQL if needed.
- Extracts CodeQL initialization logic to occur earlier in the upload flow when multiple SARIF files are detected
- Refactors the upload functions to accept and pass through a CodeQL instance parameter
- Updates all call sites to pass the CodeQL instance parameter
Reviewed Changes
Copilot reviewed 10 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/upload-sarif-action.ts | Main fix - initializes CodeQL early and passes it to upload functions, adds helper functions to find SARIF files |
| src/upload-lib.ts | Refactors upload functions to accept CodeQL parameter, extracts initialization logic to new function |
| src/init-action-post-helper.ts | Updates uploadFiles call to pass CodeQL instance |
| src/analyze-action.ts | Updates uploadFiles calls to pass CodeQL instance |
| src/init-action-post-helper.test.ts | Updates test assertion to match new function signature |
| lib/*.js | Generated JavaScript files that mirror the TypeScript changes |
| ); | ||
| } | ||
| return[]; |
CopilotAIAug 6, 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 two helper functionsfindSecuritySarifFiles andfindQualitySarifFiles have duplicated logic. Consider extracting a common function that accepts the predicate as a parameter, or creating a more general function that returns both types of files to reduce code duplication.
| return[]; | |
| asyncfunctionfindSarifFilesWithPredicate( | |
| sarifPath:string, | |
| predicate:(file:string)=>boolean, | |
| fallback:string[], | |
| ):Promise<string[]>{ | |
| if(fs.lstatSync(sarifPath).isDirectory()){ | |
| returnupload_lib.findSarifFilesInDir(sarifPath,predicate); | |
| } | |
| returnfallback; | |
| } | |
| asyncfunctionfindSecuritySarifFiles(sarifPath:string):Promise<string[]>{ | |
| returnfindSarifFilesWithPredicate( | |
| sarifPath, | |
| upload_lib.CodeScanningTarget.sarifPredicate, | |
| [sarifPath], | |
| ); | |
| } | |
| asyncfunctionfindQualitySarifFiles(sarifPath:string):Promise<string[]>{ | |
| returnfindSarifFilesWithPredicate( | |
| sarifPath, | |
| upload_lib.CodeQualityTarget.sarifPredicate, | |
| [], | |
| ); |
| }else{ | ||
| codeQL=awaitinitCodeQLForUpload(gitHubVersion,features,logger); | ||
| } | ||
| } |
CopilotAIAug 6, 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.
This CodeQL initialization logic is duplicated from the originalcombineSarifFilesUsingCLI function. Since this initialization is now handled earlier in the flow in most cases, consider adding a comment explaining when this fallback initialization is still needed to clarify the code's intent.
All the places that still call `uploadFiles` instead of `uploadSpecifiedFiles` are guaranteed to already have one.
…t be needed anymore
c5c724b to4836131Comparembg commentedAug 7, 2025
@henrymercer I rebased to resolve merge conflicts and added4836131 since the init logic in |
henrymercer left a comment
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.
Throwing out a thought here — how about uploadSpecifiedFiles takes agetCodeQL(): async () => CodeQL function? We could then haveanalyze provide its CodeQL instance, andupload-sarif provide a function that memoises CodeQL so it's only loaded once?
mbg commentedAug 7, 2025
Not a bad idea. It would probably mean throwing most of the changes in this PR out, but it would be a simpler overall change and not require us to split up |
The
upload-sarifaction can be used in workflows without aninitstep. This means that it is possible for theupload-sarifaction to be used correctly even though the CodeQL CLI has not yet been initialised. However, if we are uploading multiple SARIF files to a given endpoint, then these are first combined using the CodeQL CLI. To allow this, we currently initialise the CodeQL CLI on the fly incombineSarifFilesUsingCLIif needed for this.This was fine until#2935 where we started uploading two separate SARIF files and
combineSarifFilesUsingCLIcould appear more than once in the call path. Since the CodeQL instance was shared, this could result in the CLI being initialised twice.This PR changes the
upload-sarifaction to initialise the CLI if needed earlier on in the flow and then propagates the instance down the call path so that it can be shared between multiple calls tocombineSarifFilesUsingCLI.Merge / deployment checklist