Fix upload-sarif not uploading non-.sarif files

[mbg]

Fixes #3156.

In #3123, we inadvertently introduced a regression by using the same logic for uploading SARIF files as in the analyze Action: if a file ends in .quality.sarif, we upload it to Code Quality, and if it ends in .sarif (but not .quality.sarif), we upload it to Code Scanning.

However, that didn't account for the possibility that sarif-path is set to a file or directory containing files with other extensions (such as .json).

This PR restores this behaviour by using !analyses.CodeQuality.sarifPredicate(name) as the file predicate for Code Scanning in the upload-sarif Action.

I have also refactored upload-sarif-action slightly to move some parts to a new upload-sarif module that I have added some basic tests for.

Risk assessment

For internal use only. Please select the risk level of this change:

• Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Merge / deployment checklist

• Confirm this change is backwards compatible with existing workflows.
• Consider adding a changelog entry for this change.
• Confirm the readme and docs have been updated if necessary.

[Copilot]

Pull Request Overview

This pull request refactors the upload-sarif action to extract the core upload logic into a reusable module and fixes an issue where non-.sarif files were not being uploaded properly.

• Extracts upload logic from upload-sarif-action.ts into a dedicated upload-sarif.ts module for better separation of concerns
• Updates the Code Scanning predicate to upload files that are not intended for Code Quality (including non-.sarif files)
• Adds comprehensive test coverage for the new module with various file types and scenarios

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/upload-sarif.ts	New module containing extracted upload logic with updated file filtering
src/upload-sarif.test.ts	New test file providing comprehensive coverage for the upload module
src/upload-sarif-action.ts	Refactored to use the new upload module with simplified logic
lib/upload-sarif-action.js	Generated JavaScript code updated to match TypeScript changes

[esbena]

This LGTM semantically, but I worry about erosion that the new predicate juggling introduces.

[esbena]

I'm concerned about the addition of this argument (also, there's missing jsdoc for the new parameter).
Inside findAndUpload it is not clear that you should no longer use analysis.sarifPredicate, and that it only is subtly wrong to do so.
I'd prefer that we change the config object to have a different sarifPredicate:

Suggested change

	analyses.CodeScanning,
	// Upload anything to Code Scanning that is not intended for Code Quality.
	(name) => !analyses.CodeQuality.sarifPredicate(name),
	{
	... analyses.CodeScanning,
	// Upload anything to Code Scanning that is not intended for Code Quality.
	sarifPredicate: (name) => !analyses.CodeQuality.sarifPredicate(name)
	},

[mbg]

I'd prefer that we change the config object to have a different sarifPredicate:

I agree. I wasn't particularly happy with this extra parameter when I implemented it last night either, but it was the easiest fix. Having thought about it more over night, I reached the same conclusion that a separate configuration where sarifPredicate is overriden is better.

But there is a further issue, which you haven't flagged up as such. Using !analyses.CodeQuality.sarifPredicate(name) as sarifPredicate for the call to upload_lib.findSarifFilesInDir in findAndUpload also subtly changes the behaviour to what we had before: it now uploads all files (other than .quality.sarif) from the directory, rather than just .sarif ones (which was the previous behaviour, but isn't adequate anymore because we need to filter out the .quality.sarif ones). I think that may have been my original reasoning for the extra sarifPredicate parameter so that I could use one for the file case and another for the directory case (but then forgot and used it for both).

I'll think about a better solution.

[esbena]

This is subtly different from the sibling file finding logic in upload-lib.ts

codeql-action/src/upload-lib.ts

Lines 448 to 459 in 245e2e5

	if (fs.lstatSync(sarifPath).isDirectory()) {
	sarifFiles = findSarifFilesInDir(sarifPath, isSarif);
	if (sarifFiles.length === 0) {
	// This is always a configuration error, even for first-party runs.
	throw new ConfigurationError(
	`No SARIF files found to upload in "${sarifPath}".`,
	);
	}
	} else {
	sarifFiles = [sarifPath];
	}
	return sarifFiles;

. I wonder if we could unify them. I'm aware that upload-lib.ts was not modified in this PR, so this is an early concern about implementation divergence.

Concretely, the latter's lack of sarifPredicate(sarifPath) in the else is intentional as per your .json comment, but there's no jsdoc to document that the isSarif is unused when single files are provided..

[mbg]

I agree that we should unify things and it's been on my radar to do that / get rid of uploadFiles since we worked on #3123. This probably is as good an opportunity to do that, in combination with coming up for a good solution for the other issue here.