From a85a4b41f516f4ccf0a4a8dd3e7c828e5f0fc7c1 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Fri, 19 Jan 2024 12:07:42 +0100 Subject: [PATCH 1/3] Improve CLI error messages --- .../ql-vscode/src/codeql-cli/cli-errors.ts | 102 ++++++++++++++++++ extensions/ql-vscode/src/codeql-cli/cli.ts | 28 ++--- .../ql-vscode/src/common/vscode/commands.ts | 12 +++ .../unit-tests/codeql-cli/cli-errors.test.ts | 93 ++++++++++++++++ 4 files changed, 222 insertions(+), 13 deletions(-) create mode 100644 extensions/ql-vscode/src/codeql-cli/cli-errors.ts create mode 100644 extensions/ql-vscode/test/unit-tests/codeql-cli/cli-errors.test.ts diff --git a/extensions/ql-vscode/src/codeql-cli/cli-errors.ts b/extensions/ql-vscode/src/codeql-cli/cli-errors.ts new file mode 100644 index 00000000000..5a5b7ea57c4 --- /dev/null +++ b/extensions/ql-vscode/src/codeql-cli/cli-errors.ts @@ -0,0 +1,102 @@ +import { asError, getErrorMessage } from "../common/helpers-pure"; + +// https://docs.github.com/en/code-security/codeql-cli/using-the-advanced-functionality-of-the-codeql-cli/exit-codes +const EXIT_CODE_USER_ERROR = 2; +const EXIT_CODE_CANCELLED = 98; + +export class ExitCodeError extends Error { + constructor(public readonly exitCode: number | null) { + super(`Process exited with code ${exitCode}`); + } +} + +export class CliError extends Error { + constructor( + message: string, + public readonly stderr: string | undefined, + public readonly cause: Error, + public readonly commandDescription: string, + public readonly commandArgs: string[], + ) { + super(message); + } +} + +export function getCliError( + e: unknown, + stderr: string | undefined, + commandDescription: string, + commandArgs: string[], +): CliError { + const error = asError(e); + + if (!(error instanceof ExitCodeError) || !stderr) { + return formatCliErrorFallback( + error, + stderr, + commandDescription, + commandArgs, + ); + } + + switch (error.exitCode) { + case EXIT_CODE_USER_ERROR: { + // This is an error that we should try to format nicely + const fatalErrorIndex = stderr.lastIndexOf("A fatal error occurred: "); + if (fatalErrorIndex !== -1) { + return new CliError( + stderr.slice(fatalErrorIndex), + stderr, + error, + commandDescription, + commandArgs, + ); + } + + break; + } + case EXIT_CODE_CANCELLED: { + const cancellationIndex = stderr.lastIndexOf( + "Computation was cancelled: ", + ); + if (cancellationIndex !== -1) { + return new CliError( + stderr.slice(cancellationIndex), + stderr, + error, + commandDescription, + commandArgs, + ); + } + + break; + } + } + + return formatCliErrorFallback(error, stderr, commandDescription, commandArgs); +} + +function formatCliErrorFallback( + error: Error, + stderr: string | undefined, + commandDescription: string, + commandArgs: string[], +): CliError { + if (stderr) { + return new CliError( + stderr, + undefined, + error, + commandDescription, + commandArgs, + ); + } + + return new CliError( + getErrorMessage(error), + undefined, + error, + commandDescription, + commandArgs, + ); +} diff --git a/extensions/ql-vscode/src/codeql-cli/cli.ts b/extensions/ql-vscode/src/codeql-cli/cli.ts index 8ff704a3678..064a013fe05 100644 --- a/extensions/ql-vscode/src/codeql-cli/cli.ts +++ b/extensions/ql-vscode/src/codeql-cli/cli.ts @@ -35,6 +35,7 @@ import { LINE_ENDINGS, splitStreamAtSeparators } from "../common/split-stream"; import type { Position } from "../query-server/messages"; import { LOGGING_FLAGS } from "./cli-command"; import type { CliFeatures, VersionAndFeatures } from "./cli-version"; +import { ExitCodeError, getCliError } from "./cli-errors"; /** * The version of the SARIF format that we are using. @@ -420,7 +421,9 @@ export class CodeQLCliServer implements Disposable { stderrBuffers.push(newData); }); // Listen for process exit. - process.addListener("close", (code) => reject(code)); + process.addListener("close", (code) => + reject(new ExitCodeError(code)), + ); // Write the command followed by a null terminator. process.stdin.write(JSON.stringify(args), "utf8"); process.stdin.write(this.nullBuffer); @@ -436,19 +439,18 @@ export class CodeQLCliServer implements Disposable { } catch (err) { // Kill the process if it isn't already dead. this.killProcessIfRunning(); + // Report the error (if there is a stderr then use that otherwise just report the error code or nodejs error) - const newError = - stderrBuffers.length === 0 - ? new Error( - `${description} failed with args:${EOL} ${argsString}${EOL}${err}`, - ) - : new Error( - `${description} failed with args:${EOL} ${argsString}${EOL}${Buffer.concat( - stderrBuffers, - ).toString("utf8")}`, - ); - newError.stack += getErrorStack(err); - throw newError; + const cliError = getCliError( + err, + stderrBuffers.length > 0 + ? Buffer.concat(stderrBuffers).toString("utf8") + : undefined, + description, + args, + ); + cliError.stack += getErrorStack(err); + throw cliError; } finally { if (!silent) { void this.logger.log(Buffer.concat(stderrBuffers).toString("utf8")); diff --git a/extensions/ql-vscode/src/common/vscode/commands.ts b/extensions/ql-vscode/src/common/vscode/commands.ts index 54b009da9dc..85e45b94541 100644 --- a/extensions/ql-vscode/src/common/vscode/commands.ts +++ b/extensions/ql-vscode/src/common/vscode/commands.ts @@ -13,6 +13,8 @@ import { redactableError } from "../../common/errors"; import { UserCancellationException } from "./progress"; import { telemetryListener } from "./telemetry"; import type { AppTelemetry } from "../telemetry"; +import { CliError } from "../../codeql-cli/cli-errors"; +import { EOL } from "os"; /** * Create a command manager for VSCode, wrapping registerCommandWithErrorHandling @@ -62,6 +64,16 @@ export function registerCommandWithErrorHandling( } else { void showAndLogWarningMessage(logger, errorMessage.fullMessage); } + } else if (e instanceof CliError) { + const fullMessage = `${e.commandDescription} failed with args:${EOL}${ + e.stderr ?? e.cause + } ${e.commandArgs.join(" ")}`; + void showAndLogExceptionWithTelemetry(logger, telemetry, errorMessage, { + fullMessage, + extraTelemetryProperties: { + command: commandId, + }, + }); } else { // Include the full stack in the error log only. const fullMessage = errorMessage.fullMessageWithStack; diff --git a/extensions/ql-vscode/test/unit-tests/codeql-cli/cli-errors.test.ts b/extensions/ql-vscode/test/unit-tests/codeql-cli/cli-errors.test.ts new file mode 100644 index 00000000000..882a5fe6bd9 --- /dev/null +++ b/extensions/ql-vscode/test/unit-tests/codeql-cli/cli-errors.test.ts @@ -0,0 +1,93 @@ +import { + CliError, + ExitCodeError, + getCliError, +} from "../../../src/codeql-cli/cli-errors"; +import { EOL } from "os"; + +describe("getCliError", () => { + it("returns an error with an unknown error", () => { + const error = new Error("foo"); + + expect(getCliError(error, undefined, "bar", ["baz"])).toEqual( + new CliError("foo", undefined, error, "bar", ["baz"]), + ); + }); + + it("returns an error with an unknown error with stderr", () => { + const error = new Error("foo"); + + expect(getCliError(error, "Something failed", "bar", ["baz"])).toEqual( + new CliError("Something failed", "Something failed", error, "bar", [ + "baz", + ]), + ); + }); + + it("returns an error with an unknown error with stderr", () => { + const error = new Error("foo"); + + expect(getCliError(error, "Something failed", "bar", ["baz"])).toEqual( + new CliError("Something failed", "Something failed", error, "bar", [ + "baz", + ]), + ); + }); + + it("returns an error with an exit code error with unhandled exit code", () => { + const error = new ExitCodeError(99); // OOM + + expect(getCliError(error, "OOM!", "bar", ["baz"])).toEqual( + new CliError("OOM!", "OOM!", error, "bar", ["baz"]), + ); + }); + + it("returns an error with an exit code error with handled exit code without string", () => { + const error = new ExitCodeError(2); + + expect(getCliError(error, "Something happened!", "bar", ["baz"])).toEqual( + new CliError("Something happened!", "Something happened!", error, "bar", [ + "baz", + ]), + ); + }); + + it("returns an error with a user code error with identifying string", () => { + const error = new ExitCodeError(2); + const stderr = `Something happened!${EOL}A fatal error occurred: The query did not run successfully.${EOL}The correct columns were not present.`; + + expect(getCliError(error, stderr, "bar", ["baz"])).toEqual( + new CliError( + `A fatal error occurred: The query did not run successfully.${EOL}The correct columns were not present.`, + stderr, + error, + "bar", + ["baz"], + ), + ); + }); + + it("returns an error with a user code error with cancelled string", () => { + const error = new ExitCodeError(2); + const stderr = `Running query...${EOL}Something is happening...${EOL}Computation was cancelled: Cancelled by user`; + + expect(getCliError(error, stderr, "bar", ["baz"])).toEqual( + new CliError(stderr, stderr, error, "bar", ["baz"]), + ); + }); + + it("returns an error with a cancelled error with identifying string", () => { + const error = new ExitCodeError(98); + const stderr = `Running query...${EOL}Something is happening...${EOL}Computation was cancelled: Cancelled by user`; + + expect(getCliError(error, stderr, "bar", ["baz"])).toEqual( + new CliError( + "Computation was cancelled: Cancelled by user", + stderr, + error, + "bar", + ["baz"], + ), + ); + }); +}); From ebf06b9af64f6eb4b6ba74ba3cfe61969dc30d40 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Fri, 19 Jan 2024 12:28:04 +0100 Subject: [PATCH 2/3] Update CHANGELOG --- extensions/ql-vscode/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/extensions/ql-vscode/CHANGELOG.md b/extensions/ql-vscode/CHANGELOG.md index 9a4f3c53280..f2c7c19366c 100644 --- a/extensions/ql-vscode/CHANGELOG.md +++ b/extensions/ql-vscode/CHANGELOG.md @@ -5,6 +5,7 @@ - Enable collection of telemetry for the `codeQL.addingDatabases.addDatabaseSourceToWorkspace` setting. [#3238](https://github.com/github/vscode-codeql/pull/3238) - In the CodeQL model editor, you can now select individual method rows and save changes to only the selected rows, instead of having to save the entire library model. [#3156](https://github.com/github/vscode-codeql/pull/3156) - If you run a query without having selected a database, we show a more intuitive prompt to help you select a database. [#3214](https://github.com/github/vscode-codeql/pull/3214) +- Error messages returned from the CodeQL CLI are now less verbose and more user-friendly. [#3259](https://github.com/github/vscode-codeql/pull/3259) - The UI for browsing and running CodeQL tests has moved to use VS Code's built-in test UI. This makes the CodeQL test UI more consistent with the test UIs for other languages. This change means that this extension no longer depends on the "Test Explorer UI" and "Test Adapter Converter" extensions. You can uninstall those two extensions if they are not being used by any other extensions you may have installed. From c5e9ef15f21e001d59c59e0fb926046efffa75f6 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Mon, 29 Jan 2024 14:46:52 +0100 Subject: [PATCH 3/3] Fix order of args and stderr in error message --- extensions/ql-vscode/src/common/vscode/commands.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/src/common/vscode/commands.ts b/extensions/ql-vscode/src/common/vscode/commands.ts index 85e45b94541..3edcb2ecf92 100644 --- a/extensions/ql-vscode/src/common/vscode/commands.ts +++ b/extensions/ql-vscode/src/common/vscode/commands.ts @@ -65,9 +65,9 @@ export function registerCommandWithErrorHandling( void showAndLogWarningMessage(logger, errorMessage.fullMessage); } } else if (e instanceof CliError) { - const fullMessage = `${e.commandDescription} failed with args:${EOL}${ + const fullMessage = `${e.commandDescription} failed with args:${EOL} ${e.commandArgs.join(" ")}${EOL}${ e.stderr ?? e.cause - } ${e.commandArgs.join(" ")}`; + }`; void showAndLogExceptionWithTelemetry(logger, telemetry, errorMessage, { fullMessage, extraTelemetryProperties: {