Make all errors on an unsupported platform `ConfigurationError`s #3005

redsun82 · 2025-08-06T12:57:59Z

When running on an an unsupported platform, raise a ConfigurationError, but only if the CLI chokes on something. This leaves existing action configurations running on unsupported platforms but succeeding in the same state, but on the other hand avoids us getting flagged by errors happening in those unsupported cases.

Also, tests were added for the whole cli-errors module with help from copilot. Incidentally, this uncovered a bug where one of the regexps was not escaping square brackets while searching for [autobuild]. This bug is now fixed.

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.

henrymercer

Thanks, looks good! A couple of optional suggestions:

henrymercer · 2025-08-06T13:53:06Z

src/cli-errors.ts

+  [CliConfigErrorCategory.UnsupportedPlatform]: {
+    cliErrorMessageCandidates: [new RegExp("")],
+    precondition: isUnsupportedPlatform,
+    additionalErrorMessageToAppend:


We use this in a few other error messages to give advice about how to fix the error, but I think in this case we might want to swap the ordering, e.g. something like

"This platform/arch combination (${process.platform}/${process.arch}) is not supported by the CodeQL CLI. See https://codeql.github.com/docs/codeql-overview/system-requirements. Underlying error: "

makes sense!

henrymercer · 2025-08-06T13:53:55Z

src/cli-errors.ts

+    if (configuration.precondition && !configuration.precondition()) {
+      continue;
+    }


A potentially simpler option could be modifying wrapCliConfigurationError directly (getCliConfigCategoryIfExists is only used there and can be made private). I don't have a strong opinion about it.

mbg · 2025-08-06T15:05:43Z

src/cli-errors.ts

+    cliErrorMessageCandidates: [new RegExp("")],
+    precondition: isUnsupportedPlatform,
+    additionalErrorMessageToAppend:
+      `This platform/arch combination (${process.platform}/${process.arch}) is not supported by the CodeQL CLI. See https://codeql.github.com/docs/codeql-overview/system-requirements`,


Minor: "arch" seems a bit odd in this otherwise fairly well formatted message. I'd spell it out as "architecture".

mbg · 2025-08-06T15:12:39Z

src/cli-errors.ts

+  [CliConfigErrorCategory.UnsupportedPlatform]: {
+    cliErrorMessageCandidates: [new RegExp("")],
+    precondition: isUnsupportedPlatform,


It seems quite hacky to me to use the cliErrorsConfig mechanism and depend on the order + a new check that works differently from the the existing ones (the existing ones are all logically OR-ed, while precondition is AND-ed).

I'd say that it would probably be better to handle this differently and insert a isUnsupportedPlatform check in wrapCliConfigurationError as @henrymercer suggests. I feel more strongly about this than him.

ok, I was hesitating, you're making me lean towards that too

Tests were added with copilot, and uncovered a bug where one of the regexps looking for `[autobuild]` was not escaping the square brackets.

redsun82 · 2025-08-07T08:37:09Z

@henrymercer @mbg do you think this warrants a changelog entry?

Copilot

Pull Request Overview

This PR enhances error handling for the CodeQL Action by converting errors on unsupported platforms to ConfigurationErrors and improving the regular expression pattern matching. The changes ensure that CLI errors on unsupported platform/architecture combinations are properly categorized as configuration errors while maintaining backward compatibility for existing workflows.

Key Changes

Added unsupported platform detection that returns ConfigurationError with helpful messaging
Fixed regex pattern for Gradle build failures by properly escaping square brackets
Added comprehensive test coverage for the cli-errors module

Reviewed Changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.

File	Description
src/cli-errors.ts	Enhanced error handling with unsupported platform detection and fixed regex escaping
src/cli-errors.test.ts	Added comprehensive test suite covering all CLI error scenarios and edge cases
lib/cli-errors.js	Generated JavaScript code reflecting the TypeScript changes
lib/cli-errors.test.js	Generated JavaScript test file

Comments suppressed due to low confidence (2)

src/cli-errors.ts:322

There's a typo in the comment: 'reutrn' should be 'return'.

 * the underlying `cliError`. Otherwise, reutrn `undefined`.

src/cli-errors.ts:297

The function getCliConfigCategoryIfExists was changed from export to private visibility without being marked as deprecated first. This is a breaking change that could affect consumers of this module.

function getCliConfigCategoryIfExists(

mbg · 2025-08-07T09:18:08Z

@redsun82 I don't think a changelog entry is needed for this, since it shouldn't have any user-facing effects.

mbg

Thanks for making some changes and letting Copilot generate some tests. I have a few thoughts on the new version of this.

As already mentioned above, I don't think a changelog entry will be necessary, since we are just changing an error type and that doesn't affect users.

mbg · 2025-08-07T10:10:08Z

src/cli-errors.ts

+    return new ConfigurationError(
+      "The CodeQL CLI does not support the platform/architecture combination of " +
+        `${process.platform}/${process.arch} ` +
+        "(see https://codeql.github.com/docs/codeql-overview/system-requirements). " +


Minor: Should we add this to the DocUrl enum?

src/cli-errors.ts

mbg · 2025-08-07T10:11:39Z

src/cli-errors.ts

+    ![
+      ["linux", "x64"],
+      ["win32", "x64"],
+      ["darwin", "x64"],
+      ["darwin", "arm64"],
+    ].some(


Minor: It might be nicer if this array was a top-level constant, rather than inlined here.

mbg · 2025-08-07T10:13:14Z

src/cli-errors.ts

+  const unsupportedPlatformError = getUnsupportedPlatformError(cliError);
+  if (unsupportedPlatformError !== undefined) {
+    return unsupportedPlatformError;
+  }


This seems a bit like an improvised try/catch block. Why not just return a bool from a function that checks if the platform is supported and then throw the error here?

mbg · 2025-08-07T10:14:18Z

src/cli-errors.test.ts

+function createCommandInvocationError(
+  cmd: string,
+  args: string[],
+  exitCode: number | undefined,
+  stderr: string,
+  stdout: string = "",
+): CommandInvocationError {
+  return new CommandInvocationError(cmd, args, exitCode, stderr, stdout);
+}


Minor: This seems a bit unnecessary given it's more or less the same as CommandInvocationError

copilot's reasoning here was adding the default stdout value I think. I added that to the constructor directly

mbg · 2025-08-07T10:18:05Z

src/cli-errors.test.ts

+    Object.defineProperty(process, "platform", { value: "unsupported" });
+    Object.defineProperty(process, "arch", { value: "unsupported" });


I believe we'd normally use sinon to stub a function which returns what we want for a test. I can't find any existing uses of Object.defineProperty in our codebase.

mbg

Thanks! This looks good to me now. I added one minor comment, but feel free to ignore that.

mbg · 2025-08-07T13:58:43Z

src/actions-util.ts

    public exitCode: number | undefined,
    public stderr: string,
-    public stdout: string,
+    public stdout: string = "",


Note: I don't actually see stdout being used here, only stderr, so this shouldn't break anything.

mbg · 2025-08-07T14:00:55Z

src/cli-errors.ts

 */
 export function wrapCliConfigurationError(cliError: CliError): Error {
+  if (isUnsupportedPlatform()) {
+    return getUnsupportedPlatformError(cliError);


Minor: The getUnsupportedPlatformError function is probably not necessary and this could have been inlined here, but I don't feel strongly about it. Definitely not blocking for me.

redsun82 force-pushed the redsun82/unsupported-plat branch from fc6dafd to db0113a Compare August 6, 2025 13:01

henrymercer reviewed Aug 6, 2025

View reviewed changes

mbg requested changes Aug 6, 2025

View reviewed changes

redsun82 added 2 commits August 7, 2025 09:53

Make all errors on an unsupported platform ConfigurationErrors

7b33b61

Add tests for cli-errors and fix one bug

1cfc0c2

Tests were added with copilot, and uncovered a bug where one of the regexps looking for `[autobuild]` was not escaping the square brackets.

redsun82 force-pushed the redsun82/unsupported-plat branch from db0113a to 1cfc0c2 Compare August 7, 2025 07:58

redsun82 marked this pull request as ready for review August 7, 2025 08:37

redsun82 requested a review from a team as a code owner August 7, 2025 08:37

Copilot AI review requested due to automatic review settings August 7, 2025 08:37

Copilot AI reviewed Aug 7, 2025

View reviewed changes

mbg reviewed Aug 7, 2025

View reviewed changes

Address review

bb56324

redsun82 requested a review from mbg August 7, 2025 13:45

Merge branch 'main' into redsun82/unsupported-plat

739fb03

mbg approved these changes Aug 7, 2025

View reviewed changes

redsun82 merged commit 588ff73 into main Aug 7, 2025
282 checks passed

redsun82 deleted the redsun82/unsupported-plat branch August 7, 2025 14:24

github-actions bot mentioned this pull request Aug 7, 2025

Merge main into releases/v3 #3012

Merged

8 tasks

		Object.defineProperty(process, "platform", { value: "unsupported" });
		Object.defineProperty(process, "arch", { value: "unsupported" });

Make all errors on an unsupported platform ConfigurationErrors #3005

Make all errors on an unsupported platform ConfigurationErrors #3005

Uh oh!

Conversation

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

Uh oh!

Merge / deployment checklist

Uh oh!

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

redsun82 commented Aug 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

mbg commented Aug 7, 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Make all errors on an unsupported platform `ConfigurationError`s #3005

Make all errors on an unsupported platform `ConfigurationError`s #3005

redsun82 commented Aug 6, 2025 •

edited

Loading