Produce separate SARIF file for `quality-queries` alerts #2935

mbg · 2025-06-16T15:19:15Z

Follows on from #2917.

This PR modifies the action to produce separate SARIF files for code quality queries (as specified as arguments to the quality-queries input) and uploads them to the code quality API. The approach here is that:

We make an additional call to database interpret-results for queries that were specified as arguments to the quality-queries input (if any). This results in SARIF files for those queries.
We then upload them to the CQ API endpoint.

Notes

The changes aim to be as backwards-compatible as possible. That includes:
- We don't (yet) modify the existing SARIF files that are uploaded to the code scanning API to exclude results for quality-queries alerts.
- SARIF files are output to the same place as before (e.g. as opposed to having separate subdirectories for security and quality SARIFs)
We don't support query-filter options with respect to code quality results.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

src/upload-lib.ts

+    const response = await client.request(target, {
+      owner: repositoryNwo.owner,
+      repo: repositoryNwo.repo,
+      data: payload,
+    });


Copilot

Pull Request Overview

This PR enables separate generation and upload of SARIF files for code-quality queries specified via the quality-queries input, in addition to the existing code-scanning SARIF flow.

Introduces SARIF_UPLOAD_TARGET enum and UploadTarget interface to distinguish code scanning vs. code quality uploads.
Extends file discovery (findSarifFilesInDir, getSarifFilePaths) and upload logic (uploadFiles, uploadPayload, validateUniqueCategory) to filter and handle .quality.sarif files.
Updates analyze.ts/analyze-action.ts to produce and upload quality SARIFs when quality-queries are provided, and adjusts CI workflows to upload/check both SARIF artifacts.

Reviewed Changes

Copilot reviewed 12 out of 17 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/upload-lib.ts	Added enum and targets for separate SARIF uploads, filtering logic for `.quality.sarif`.
src/upload-lib.test.ts	Added tests for filtering `.quality.sarif` files and `validateUniqueCategory` prefix.
src/analyze.ts	Introduced default query suites and `resolveQuerySuiteAlias`; generate quality SARIF.
src/analyze.test.ts	Added tests for `resolveQuerySuiteAlias`.
src/analyze-action.ts	Uploads quality SARIF and sets `quality-sarif-id` output when `quality-queries` present.
pr-checks/checks/quality-queries.yml	CI updates to upload security vs. quality SARIF artifacts and adjust checks.

Comments suppressed due to low confidence (2)

src/upload-lib.ts:427

The getSarifFilePaths function bypasses the isSarif filter when sarifPath is a single file. This can lead to uploading unwanted files (e.g., a .quality.sarif when targeting code-scanning). Apply isSarif to the single-file case or throw if it does not match.

    sarifFiles = [sarifPath];

pr-checks/checks/quality-queries.yml:29

[nitpick] The CI step now only checks config properties in the quality SARIF, omitting the original security SARIF check. Consider adding a separate check for ${{ runner.temp }}/results/javascript.sarif to ensure both artifacts are validated.

      SARIF_PATH: "${{ runner.temp }}/results/javascript.quality.sarif"

esbena

Nice, and pleasantly surgical.

I think this does the right thing, except for a missing quality.sarif predicate in the very end.

Other than that it's mostly nits, and some concerns about maintenance in the long term. We have already discussed them elsewhere. Perhaps we should update the discussion document with our current choice for query resolution.

esbena · 2025-06-25T21:14:43Z

src/analyze.ts

+ */
+export function resolveQuerySuiteAlias(
+  language: Language,
+  query: string,


Nit: I think should be named suite instead of query, but if there's precedence for using the query name elsewhere, then just keep it.

esbena · 2025-06-25T21:17:27Z

src/analyze.ts

+export const defaultSuites: Set<string> = new Set([
+  "security-experimental",
+  "security-extended",
+  "security-and-quality",
+  "code-quality",
+  "code-scanning",
+]);


This is my least favorite part of the PR. As it leads to double maintenance.

It would be great if the same resolution that the ordinary .queries uses could be reused here. In the interest of time, we don't need to do it right now though. Could you perhaps create an issue with a list of things to follow up on later?

esbena · 2025-06-25T21:20:39Z

src/codeql.ts

+  databaseRunQueries(
+    databasePath: string,
+    flags: string[],
+    queries?: string[],


this deserves a jsdoc comment.
I'm guessing that the semantics is that she CLI will magically look up the query suite to use if nothing explicitly is provided by the user, and not that it will run zero queries.

esbena · 2025-06-25T21:23:46Z

src/upload-lib.ts

@@ -306,12 +306,19 @@ function getAutomationID(
  return api.computeAutomationID(analysis_key, environment);
 }

+// Enumerates API endpoints that accept SARIF files.
+export enum SARIF_UPLOAD_TARGET {


Nit: SARIF_UPLOAD_ENDPOINT is a more standard naming scheme for <method> <path> strings like this.

esbena · 2025-06-25T21:27:36Z

src/upload-lib.ts

+export const qualityIsSarif = (name: string) => name.endsWith(".quality.sarif");
+export const defaultIsSarif = (name: string) =>
+  name.endsWith(".sarif") && !qualityIsSarif(name);


could we move these definitions into UploadTarget instances below, so you always access them through their associated target. I'd like a very strong coupling between the components of such targets to avoid a fragment being used wrongly.

That is: I love seeing sentinelPrefix: string = CodeScanningTarget.sentinelPrefix, below rather than a direct reference to a codeScanningSentinelPrefix

esbena · 2025-06-25T21:33:33Z

src/upload-lib.ts

@@ -567,6 +581,30 @@ export function buildPayload(
  return payloadObj;
 }

+// Represents configurations for different services that we can upload SARIF to.
+export interface UploadTarget {


regarding the target/endpoint comment above: this is a much better place to call something target. I'd probably prefer SarifUploadConfig but that is splitting hairs.

esbena · 2025-06-25T21:34:40Z

src/upload-lib.ts

@@ -567,6 +581,30 @@ export function buildPayload(
  return payloadObj;
 }

+// Represents configurations for different services that we can upload SARIF to.
+export interface UploadTarget {


I really like to see this config pattern used here. Good choice.

esbena · 2025-06-25T21:38:44Z

src/upload-lib.ts

+  sentinelPrefix: string;
+}
+
+// Represents the Code Scanning upload target.


Stray thought. I have seen both line comments and jsdoc comments on your new functions. Is it deliberate that you do not use jsdoc here? If yes, OK.

esbena · 2025-06-25T21:41:13Z

src/upload-sarif-action.ts

      features,
      logger,
    );
    core.setOutput("sarif-id", uploadResult.sarifID);

+    // If there are `.quality.sarif` files in `sarifPath`, then upload those to the code quality service.
+    const qualitySarifFiles = upload_lib.getSarifFilePaths(sarifPath);


Bug: getSarifFilePaths(sarifPath); is this missing a quality specialization through some argument, or is there some hidden state elsewhere?

I think this should be getSarifFilePaths(sarifPath, upload_lib.CodeQualityTarget.sarifFilter),

esbena · 2025-06-25T21:46:34Z

src/upload-lib.ts

+export interface UploadTarget {
+  name: string;
+  target: SARIF_UPLOAD_TARGET;
+  sarifFilter: (name: string) => boolean;


Nit: a filter is typically a function of Array -> SubsetArray, whereas this is a predicate. The relation being that a filter is instantiated with a predicate.

mbg force-pushed the mbg/interpret-cq-results branch 2 times, most recently from 87b6687 to fe76653 Compare June 17, 2025 13:37

github-advanced-security bot found potential problems Jun 17, 2025

View reviewed changes

mbg added 8 commits June 23, 2025 18:19

Interpret results for quality queries and store as separate SARIF file

3963bf4

Upload both SARIF files in quality-queries check

aba8788

Check SARIF with quality results for expected configuration

3a7544e

Resolve code-quality alias

320f7b0

Add ability to use different filters in findSarifFilesInDir

22444a6

Upload quality SARIFs to CQ endpoint

45b3bec

Allow the same category once for each type of upload

f183422

Fix getSarifFilePaths not using right filter

6abacdb

mbg force-pushed the mbg/interpret-cq-results branch from 2bf7322 to 24e4f58 Compare June 23, 2025 17:35

Support all default query suites and resolve them

f7fbaa0

mbg force-pushed the mbg/interpret-cq-results branch from 7e59f77 to f7fbaa0 Compare June 24, 2025 12:09

mbg added 4 commits June 24, 2025 13:24

Add test for modified findSarifFilesInDir

5189159

Add test for modified validateUniqueCategory

af32bc6

Add test for resolveQuerySuiteAlias

9b9286a

Add some more comments

86f47e8

mbg marked this pull request as ready for review June 24, 2025 13:08

mbg requested a review from a team as a code owner June 24, 2025 13:08

mbg requested a review from Copilot June 24, 2025 13:09

Copilot AI reviewed Jun 24, 2025

View reviewed changes

Upload .quality.sarif files to CQ service in upload-sarif action

2c76207

mbg force-pushed the mbg/interpret-cq-results branch 2 times, most recently from 3393b4e to 81969d7 Compare June 25, 2025 13:24

Prototyping adding quality queries when running queries

e382508

mbg force-pushed the mbg/interpret-cq-results branch 2 times, most recently from 4088317 to b403ba2 Compare June 25, 2025 13:38

Fix config-queries.qls location

79049d9

mbg force-pushed the mbg/interpret-cq-results branch from b403ba2 to 79049d9 Compare June 25, 2025 13:42

esbena reviewed Jun 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Produce separate SARIF file for `quality-queries` alerts #2935

Produce separate SARIF file for `quality-queries` alerts #2935

Uh oh!

mbg commented Jun 16, 2025 •

edited

Loading

Uh oh!

Check warning

Copilot AI left a comment

Uh oh!

esbena left a comment

Uh oh!

esbena Jun 25, 2025

Uh oh!

esbena Jun 25, 2025

Uh oh!

esbena Jun 25, 2025

Uh oh!

esbena Jun 25, 2025

Uh oh!

esbena Jun 25, 2025

Uh oh!

esbena Jun 25, 2025

Uh oh!

esbena Jun 25, 2025

Uh oh!

esbena Jun 25, 2025

Uh oh!

esbena Jun 25, 2025

Uh oh!

esbena Jun 25, 2025

Uh oh!

Uh oh!

Produce separate SARIF file for quality-queries alerts #2935

Are you sure you want to change the base?

Produce separate SARIF file for quality-queries alerts #2935

Uh oh!

Conversation

mbg commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Merge / deployment checklist

Uh oh!

Check warning

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Produce separate SARIF file for `quality-queries` alerts #2935

Produce separate SARIF file for `quality-queries` alerts #2935

mbg commented Jun 16, 2025 •

edited

Loading