Introduce `CODEQL_ACTION_SKIP_SARIF_UPLOAD` #3180

redsun82 · 2025-10-06T13:10:32Z

This triggers a subset of the behavior of CODEQL_ACTION_TEST_MODE, specifically just skipping the SARIF upload step. This is required for our internal testing where we want the SARIF file (via CODEQL_ACTION_DUMP_SARIF_DIR) but don't want to actually upload it, but we don't want the rest of the behaviour of CODEQL_ACTION_TEST_MODE that is specific for codeql-action own CI checks.

This is a fresh take at #3089, with less impact on the codebase.

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.
High risk: Changes are not fully under feature flags, have limited visibility and/or cannot be tested outside of production.

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.

This triggers a subset of the behavior of `CODEQL_ACTION_TEST_MODE`, specifically just skipping the SARIF upload step. This is required for our internal testing where we want the SARIF file (via `CODEQL_ACTION_DUMP_SARIF_DIR`) but don't want to actually upload it, but we don't want the rest of the behaviour of `CODEQL_ACTION_TEST_MODE` that is specific for `codeql-action` own CI checks.

Copilot

Pull Request Overview

This PR introduces a new environment variable CODEQL_ACTION_SKIP_SARIF_UPLOAD that provides more granular control over SARIF upload behavior. It allows skipping SARIF uploads without enabling full test mode, which is useful for internal testing scenarios where SARIF files are needed locally but should not be uploaded to GitHub.

Key changes:

Adds new environment variable CODEQL_ACTION_SKIP_SARIF_UPLOAD for selective SARIF upload control
Introduces shouldSkipSarifUpload() function that combines test mode and the new skip flag
Updates upload logic and messaging to use the new function instead of direct test mode checks

Reviewed Changes

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

Show a summary per file

File	Description
src/environment.ts	Adds the new `SKIP_SARIF_UPLOAD` environment variable enum
src/util.ts	Introduces `shouldSkipSarifUpload()` function and updates test mode documentation
src/upload-lib.ts	Updates upload logic to use new function and improves messaging
src/upload-sarif-action.ts	Updates wait-for-processing logic to use new function
src/init-action-post-helper.ts	Updates failed SARIF upload logic to use new function
lib/*.js	Generated JavaScript code corresponding to TypeScript changes

src/util.ts

Co-authored-by: Copilot <[email protected]>

esbena

LGTM, but Michael should have the say.

esbena · 2025-10-06T13:23:06Z

src/upload-lib.ts


-  // If in test mode we don't want to upload the results
-  if (util.isInTestMode()) {
+  // If in test mode we don't want to upload the results,


this comment is a bit stale now, ditto at the other isInTestMode/shouldSkipSarifUpload switch.

mbg

Thanks, this is generally much neater! I've added a few minor comments.

It is probably worth adding a test case for this.

mbg · 2025-10-06T13:15:29Z

src/environment.ts

+
+  /**
+   * Whether to skip uploading SARIF results to GitHub. Intended for testing purposes.
+   * This setting is implied but is more specific than `CODEQL_ACTION_TEST_MODE`.


Minor: the wording here isn't great.

Suggested change

* This setting is implied but is more specific than `CODEQL_ACTION_TEST_MODE`.

* This setting is more specific than `CODEQL_ACTION_TEST_MODE`, which also implies this option.

mbg · 2025-10-06T13:21:16Z

src/upload-lib.ts

-    logger.info(
-      `In test mode. Results are not uploaded. Saving to ${payloadSaveFile}`,
-    );
+    logger.info(`SARIF upload disabled. Saving to ${payloadSaveFile}`);


Minor: This no longer explains why the SARIF upload is disabled. A simple fix might be wording it like this:

Suggested change

logger.info(`SARIF upload disabled. Saving to ${payloadSaveFile}`);

logger.info(`SARIF upload disabled with an environment variable. Saving to ${payloadSaveFile}`);

A better fix would be to include the responsible environment variable, perhaps with:

Suggested change

logger.info(`SARIF upload disabled. Saving to ${payloadSaveFile}`);

logger.info(`SARIF upload disabled by `${ isInTestMode() ? EnvVar.TEST_MODE : EnvVar.SKIP_SARIF_UPLOAD }`. Saving to ${payloadSaveFile}`);

src/util.ts

redsun82 · 2025-10-06T13:53:24Z

It is probably worth adding a test case for this

@mbg: how would you go on about this?

the PR checks set CODEQL_ACTION_TEST_MODE, which "hides" this new behaviour. Maybe a PR check could unset that and set CODEQL_ACTION_SKIP_SARIF_UPLOAD instead? Wouldn't there be some other unwanted effects in the PR checks though?
on the unit test side, there's currently no unit tests for uploadSpecifiedFiles, and CODEQL_ACTION_TEST_MODE is not covered at all in there. Adding tests for uploadSpecifiedFiles would probably be nice, but maybe a tad too much for the scope of this PR?

mbg · 2025-10-06T14:05:53Z

src/util.ts

+export function getSarifUploadSkipReason(): string | null {
+  if (isInTestMode()) {
+    return `SARIF upload is disabled via ${EnvVar.TEST_MODE}`;
+  }
+  if (process.env[EnvVar.SKIP_SARIF_UPLOAD] === "true") {
+    return `SARIF upload is disabled via ${EnvVar.SKIP_SARIF_UPLOAD}`;
+  }
+  return null;
+}


I had considered something like this when I wrote my comment, but wasn't too keen on it. I also don't love using the human-readable reason phrase to mean true for enablement purposes. The previous version with the boolean function was preferable.

Also, in general, we don't use null in the codeql-action codebase (unless it comes to us from outside the codebase) and would go for undefined here.

I was trying to avoid something like

logger.info(`SARIF upload disabled by `${ isInTestMode() ? EnvVar.TEST_MODE : EnvVar.SKIP_SARIF_UPLOAD }`. Saving to ${payloadSaveFile}`);

as I didn't want to expose that util implementation detail. So I think I'll just go with your first proposal for a generic with an environment variable. The users of those variables (us) probably are savvy enough for that to be enough for them.

mbg · 2025-10-06T14:09:12Z

how would you go on about this?
on the unit test side, there's currently no unit tests [..]

Hmm yes. Unit tests for uploadPayload would be best. I don't think it would be too hard to set this up with mocks for the IO operations (fs.writeFileSync and client.request) and checking which ones were called for given permutations of the environment variables. That said, if there aren't any unit tests here already, then don't feel obligated to set this up as part of this PR.

redsun82 · 2025-10-06T14:58:36Z

how would you go on about this?
on the unit test side, there's currently no unit tests [..]

Hmm yes. Unit tests for uploadPayload would be best. I don't think it would be too hard to set this up with mocks for the IO operations (fs.writeFileSync and client.request) and checking which ones were called for given permutations of the environment variables. That said, if there aren't any unit tests here already, then don't feel obligated to set this up as part of this PR.

Let's make that into a follow-up PR. I'll try to see what copilot can do there.

This reverts commit 680b070.

mbg

Looks like you accidentally reverted one of the changes we wanted to keep. Also two other minor suggestions for wording improvements.

Otherwise this LGTM and I am happy to approve.

mbg · 2025-10-07T14:00:33Z

src/environment.ts

  /**
   * Whether to skip uploading SARIF results to GitHub. Intended for testing purposes.
-   * This setting is implied by `CODEQL_ACTION_TEST_MODE`, but is more specific.
+   * This setting is implied by but is more specific than `CODEQL_ACTION_TEST_MODE`.


Did you intend to revert this?

mbg · 2025-10-07T14:03:15Z

src/upload-lib.ts

    );
    logger.info(
-      `In test mode. Results are not uploaded. Saving to ${payloadSaveFile}`,
+      `SARIF upload disabled via environment variable. Saving to ${payloadSaveFile}`,


Minor:

Suggested change

`SARIF upload disabled via environment variable. Saving to ${payloadSaveFile}`,

`SARIF upload disabled by an environment variable. Saving to ${payloadSaveFile}`,

mbg · 2025-10-07T14:03:39Z

src/upload-sarif-action.ts

-      core.debug("In test mode. Waiting for processing is disabled.");
+    if (shouldSkipSarifUpload()) {
+      core.debug(
+        "SARIF upload disabled via environment variable. Waiting for processing is disabled.",


Suggested change

"SARIF upload disabled via environment variable. Waiting for processing is disabled.",

"SARIF upload disabled by an environment variable. Waiting for processing is disabled.",

redsun82 · 2025-10-07T15:18:56Z

@mbg thanks for the review! I think I addressed everything. I also opened a follow-up with unit tests: #3185 (with a little help from copilot)

mbg

Thanks for fixing those minor issues!

redsun82 requested a review from a team as a code owner October 6, 2025 13:10

redsun82 requested review from Copilot and mbg October 6, 2025 13:10

Copilot AI reviewed Oct 6, 2025

View reviewed changes

src/util.ts Outdated Show resolved Hide resolved

redsun82 and others added 2 commits October 6, 2025 15:22

Update src/util.ts

882667e

Co-authored-by: Copilot <[email protected]>

Clarify comment about SKIP_SARIF_UPLOAD setting

11e4034

esbena reviewed Oct 6, 2025

View reviewed changes

mbg reviewed Oct 6, 2025

View reviewed changes

Specify reason for skipping SARIF upload in logs

680b070

mbg reviewed Oct 6, 2025

View reviewed changes

redsun82 added 4 commits October 6, 2025 16:59

Revert "Specify reason for skipping SARIF upload in logs"

25c8db9

This reverts commit 680b070.

Tweak SARIF skipping logs

00a6e13

Merge branch 'main' into redsun82/skip-sarif-upload

5dfb610

Remove unneeded comment

86b2ad6

mbg reviewed Oct 7, 2025

View reviewed changes

redsun82 added 2 commits October 7, 2025 17:17

Address review

1b09eb4

Merge branch 'main' into redsun82/skip-sarif-upload

df65651

redsun82 enabled auto-merge October 7, 2025 15:18

redsun82 added 2 commits October 8, 2025 09:34

Tweak comment

d05f225

Merge branch 'main' into redsun82/skip-sarif-upload

1707898

mbg approved these changes Oct 8, 2025

View reviewed changes

redsun82 merged commit 8301b8b into main Oct 8, 2025
242 checks passed

redsun82 deleted the redsun82/skip-sarif-upload branch October 8, 2025 10:09

mbg mentioned this pull request Oct 8, 2025

Add unit tests for uploadSpecifiedFiles #3185

Open

	* This setting is implied but is more specific than `CODEQL_ACTION_TEST_MODE`.
	* This setting is more specific than `CODEQL_ACTION_TEST_MODE`, which also implies this option.

	logger.info(`SARIF upload disabled. Saving to ${payloadSaveFile}`);
	logger.info(`SARIF upload disabled with an environment variable. Saving to ${payloadSaveFile}`);

	logger.info(`SARIF upload disabled. Saving to ${payloadSaveFile}`);
	logger.info(`SARIF upload disabled by `${ isInTestMode() ? EnvVar.TEST_MODE : EnvVar.SKIP_SARIF_UPLOAD }`. Saving to ${payloadSaveFile}`);

	`SARIF upload disabled via environment variable. Saving to ${payloadSaveFile}`,
	`SARIF upload disabled by an environment variable. Saving to ${payloadSaveFile}`,

	"SARIF upload disabled via environment variable. Waiting for processing is disabled.",
	"SARIF upload disabled by an environment variable. Waiting for processing is disabled.",

Introduce CODEQL_ACTION_SKIP_SARIF_UPLOAD #3180

Introduce CODEQL_ACTION_SKIP_SARIF_UPLOAD #3180

Uh oh!

Conversation

redsun82 commented Oct 6, 2025

Risk assessment

Merge / deployment checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbg 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!

Uh oh!

redsun82 commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redsun82 Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbg commented Oct 6, 2025

Uh oh!

redsun82 commented Oct 6, 2025

Uh oh!

mbg 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!

redsun82 commented Oct 7, 2025

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Introduce `CODEQL_ACTION_SKIP_SARIF_UPLOAD` #3180

Introduce `CODEQL_ACTION_SKIP_SARIF_UPLOAD` #3180

redsun82 commented Oct 6, 2025 •

edited

Loading

redsun82 Oct 6, 2025 •

edited

Loading