Skip to content

Conversation

mbg
Copy link
Member

@mbg mbg commented Sep 18, 2025

In #3064, we overlooked that upload_lib.uploadFiles is called in the upload-sarif Action, which will fail if it cannot find any SARIF files matching the predicate it is given.

For a Code Quality only analysis, there are no Code Scanning SARIF files and the call to upload_lib.uploadFiles therefore fails before the Code Quality SARIF file(s) can be uploaded.

This PR refactors the upload-sarif Action so that there's common code for Code Scanning and Code Quality to upload either one file or several matching files.

There are some optional, other changes in this PR as well, which I can remove if needed:

  • The UploadStatusReport values for each SARIF upload are now combined for telemetry if both analysis kinds are enabled. Otherwise, we use the respective UploadStatusReport.
  • I have added a new sarif-ids output to the upload-sarif Action, which contains a stringified JSON object with details of the SARIF ids that we uploaded to different endpoints. Initially I went for just a comma-separated list of IDs, but I thought it would be useful to know which ID is for which service.

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.

@mbg mbg requested a review from a team as a code owner September 18, 2025 13:58
@mbg mbg requested review from Copilot and henrymercer September 18, 2025 13:58
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an issue where the upload-sarif Action fails when there are no Code Scanning SARIF files, specifically for code quality-only analysis. It refactors the upload process to handle both Code Scanning and Code Quality SARIF files more gracefully and adds new outputs for tracking uploaded SARIF IDs.

  • Refactors upload logic to use a common findAndUpload function for both analysis types
  • Adds new sarif-ids output containing a JSON object with SARIF IDs for each analysis type
  • Combines upload status reports when both analysis kinds are enabled
  • Updates test configuration to test code quality-only scenarios

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
upload-sarif/action.yml Updates output descriptions and adds new sarif-ids output
src/upload-sarif-action.ts Refactors upload logic with new findAndUpload function and combined status reporting
src/upload-lib.ts Adds combineSarifUploadResults function to merge upload status reports
pr-checks/checks/upload-quality-sarif.yml Updates test to use code-quality only and validates new output
lib/upload-sarif-action.js Generated JavaScript code reflecting TypeScript changes
lib/upload-lib.js Generated JavaScript code with new export for combine function
.github/workflows/__upload-quality-sarif.yml Auto-generated workflow file updated for code-quality only testing

@mbg mbg force-pushed the mbg/fix/upload-sarif-cq-only branch from 00415e9 to 79b45c8 Compare September 18, 2025 14:03
@mbg mbg force-pushed the mbg/fix/upload-sarif-cq-only branch from 6d2c7db to bca5bc4 Compare September 18, 2025 15:29
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you motivate the Disable cpp in upload-quality-sarif check commit?

await sendSuccessStatusReport(startedAt, uploadResult.statusReport, logger);
await sendSuccessStatusReport(
startedAt,
uploadResult?.statusReport || {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it convention to add the empty object to these statuses by default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this was an intermediate commit and the {} just made the type checker happy until I replaced it with the statusReport from the Code Quality upload or the combination of both uploads.


The three properties of UploadStatusReport are all of type number | undefined, but the UploadStatusReport object itself is required here by sendSuccessStatusReport.

Previously, with only Code Scanning, we only would have reached sendSuccessStatusReport if the SARIF upload to Code Scanning is successful. In that case, the UploadStatusReport object always would have been populated.

Now we also have Code Quality and Code Scanning + Code Quality. So the status report should include the results of the respective upload(s).

Comment on lines 1062 to 1077
for (const report of reports) {
if (report !== undefined) {
result.num_results_in_sarif = addOptional(
result.num_results_in_sarif,
report.num_results_in_sarif,
);
result.raw_upload_size_bytes = addOptional(
result.raw_upload_size_bytes,
report.raw_upload_size_bytes,
);
result.zipped_upload_size_bytes = addOptional(
result.zipped_upload_size_bytes,
report.zipped_upload_size_bytes,
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too ad hoc for my taste. Once we add a new numeric field to the type, it will get dropped when going through this function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I.e. something like

  for (const report of reports) {
    if (!report) continue;
    for (const key of Object.keys(report) as (keyof UploadStatusReport)[]) {
      const value = report[key];
      if (typeof value !== "number") continue; // Skip non-numeric / undefined
      result[key] = ((result[key] ?? 0) as number) + value;
    }
  }

Conversely, we now risk adding things that can't be added...

Ultimately, we could do some typescript magic where we'd force ourselves to list every numerically typed property as being added or ignored, but that is complex.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do something like that. Perhaps it is best if we leave sorting out the telemetry for a separate change so that we can think about this independently from the bug fix -- see my thoughts in the PR description and in #3123 (comment)

@mbg
Copy link
Member Author

mbg commented Sep 19, 2025

Can you motivate the Disable cpp in upload-quality-sarif check commit?

@esbena C++ has no queries in the code-quality suite and the CLI fails as a result. See https://github.com/github/codeql-action/actions/runs/17831231939/job/50696560032?pr=3123#step:5:892

esbena
esbena previously approved these changes Sep 19, 2025
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it is best if we leave sorting out the telemetry for a separate change so that we can think about this independently from the bug fix

OK and conditional approve if we do that change elsewhere.

mbg added 5 commits September 19, 2025 16:17
Unlike `sarif-id` which is for the single Code Scanning SARIF id, `sarif-ids` contains stringified JSON object with details of all SARIF ids.
@mbg mbg force-pushed the mbg/fix/upload-sarif-cq-only branch from bca5bc4 to db37d92 Compare September 19, 2025 15:17
@mbg
Copy link
Member Author

mbg commented Sep 19, 2025

I've removed the telemetry changes, other than that in the CQ-only case we send an empty UploadStatusReport. I think that's better than setting the fields to 0 since it allows us to tell apart status reports for CS/CS+CQ that have the data for CS from those that are for CQ-only and have no data.

@mbg mbg requested a review from esbena September 19, 2025 15:22
@mbg mbg merged commit 0337c4c into main Sep 19, 2025
299 checks passed
@mbg mbg deleted the mbg/fix/upload-sarif-cq-only branch September 19, 2025 17:48
@github-actions github-actions bot mentioned this pull request Sep 25, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants