Merge branch 'main' into henrymercer/report-failed-runs

src/analyze-action.ts

@@ -322,8 +322,6 @@ async function run() {
       core.setFailed(error.message);
     }
 
-    console.log(error);
-
     if (error instanceof CodeQLAnalysisError) {
       const stats = { ...error.queriesStatusReport };
       await sendStatusReport(
@@ -398,7 +396,6 @@ async function runWrapper() {
     await runPromise;
   } catch (error) {
     core.setFailed(`analyze action failed: ${error}`);
-    console.log(error);
   }
   await checkForTimeout();
 }

src/codeql.test.ts

@@ -3,6 +3,7 @@ import * as path from "path";
 
 import * as toolrunner from "@actions/exec/lib/toolrunner";
 import * as toolcache from "@actions/tool-cache";
+import * as safeWhich from "@chrisgavin/safe-which";
 import test, { ExecutionContext } from "ava";
 import del from "del";
 import * as yaml from "js-yaml";
@@ -442,6 +443,8 @@ test("databaseInterpretResults() does not set --sarif-add-query-help for 2.7.0",
   const runnerConstructorStub = stubToolRunnerConstructor();
   const codeqlObject = await codeql.getCodeQLForTesting();
   sinon.stub(codeqlObject, "getVersion").resolves("2.7.0");
+  // safeWhich throws because of the test CodeQL object.
+  sinon.stub(safeWhich, "safeWhich").resolves("");
   await codeqlObject.databaseInterpretResults(
     "",
     [],
@@ -462,6 +465,8 @@ test("databaseInterpretResults() sets --sarif-add-query-help for 2.7.1", async (
   const runnerConstructorStub = stubToolRunnerConstructor();
   const codeqlObject = await codeql.getCodeQLForTesting();
   sinon.stub(codeqlObject, "getVersion").resolves("2.7.1");
+  // safeWhich throws because of the test CodeQL object.
+  sinon.stub(safeWhich, "safeWhich").resolves("");
   await codeqlObject.databaseInterpretResults(
     "",
     [],
@@ -483,6 +488,8 @@ test("databaseInitCluster() without injected codescanning config", async (t) =>
     const runnerConstructorStub = stubToolRunnerConstructor();
     const codeqlObject = await codeql.getCodeQLForTesting();
     sinon.stub(codeqlObject, "getVersion").resolves("2.8.1");
+    // safeWhich throws because of the test CodeQL object.
+    sinon.stub(safeWhich, "safeWhich").resolves("");
 
     const thisStubConfig: Config = {
       ...stubConfig,
@@ -865,6 +872,8 @@ test("databaseInterpretResults() sets --sarif-add-baseline-file-info when featur
   // The version of CodeQL is checked separately to determine feature enablement, and does not
   // otherwise impact this test, so set it to 0.0.0.
   sinon.stub(codeqlObject, "getVersion").resolves("0.0.0");
+  // safeWhich throws because of the test CodeQL object.
+  sinon.stub(safeWhich, "safeWhich").resolves("");
   await codeqlObject.databaseInterpretResults(
     "",
     [],
@@ -890,6 +899,8 @@ test("databaseInterpretResults() does not set --sarif-add-baseline-file-info if
   // The version of CodeQL is checked upstream to determine feature enablement, so it does not
   // affect this test.
   sinon.stub(codeqlObject, "getVersion").resolves("0.0.0");
+  // safeWhich throws because of the test CodeQL object.
+  sinon.stub(safeWhich, "safeWhich").resolves("");
   await codeqlObject.databaseInterpretResults(
     "",
     [],

src/codeql.ts

@@ -1024,7 +1024,7 @@ async function getCodeQLForCmd(
       if (querySuitePath) {
         codeqlArgs.push(querySuitePath);
       }
-      await runTool(cmd, codeqlArgs);
+      await toolrunnerErrorCatcher(cmd, codeqlArgs, errorMatchers);
     },
     async databaseInterpretResults(
       databasePath: string,
@@ -1067,7 +1067,12 @@ async function getCodeQLForCmd(
         codeqlArgs.push(...querySuitePaths);
       }
       // capture stdout, which contains analysis summaries
-      return await runTool(cmd, codeqlArgs);
+      const returnState = await toolrunnerErrorCatcher(
+        cmd,
+        codeqlArgs,
+        errorMatchers
+      );
+      return returnState.stdout;
     },
     async databasePrintBaseline(databasePath: string): Promise<string> {
       const codeqlArgs = [
@@ -1271,11 +1276,16 @@ async function runTool(cmd: string, args: string[] = []) {
   const exitCode = await new toolrunner.ToolRunner(cmd, args, {
     listeners: {
       stdout: (data: Buffer) => {
-        output += data.toString();
+        output += data.toString("utf8");
       },
       stderr: (data: Buffer) => {
-        const toRead = Math.min(maxErrorSize - error.length, data.length);
-        error += data.toString("utf8", 0, toRead);
+        let readStartIndex = 0;
+        // If the error is too large, then we only take the last 20,000 characters
+        if (data.length - maxErrorSize > 0) {
+          // Eg: if we have 20,000 the start index should be 2.
+          readStartIndex = data.length - maxErrorSize + 1;
+        }
+        error += data.toString("utf8", readStartIndex);
       },
     },
     ignoreReturnCode: true,

src/error-matcher.test.ts

@@ -20,6 +20,15 @@ test("noSourceCodeFound matches against example javascript output", async (t) =>
   );
 });
 
+test("fatalError matches against example log output", async (t) => {
+  t.assert(
+    testErrorMatcher(
+      "fatalError",
+      "A fatal error occurred: Could not process query metadata for test-query.ql"
+    )
+  );
+});
+
 function testErrorMatcher(matcherName: string, logSample: string): boolean {
   if (!(matcherName in namedMatchersForTesting)) {
     throw new Error(`Unknown matcher ${matcherName}`);

src/error-matcher.ts

@@ -18,6 +18,10 @@ export const namedMatchersForTesting: { [key: string]: ErrorMatcher } = {
       "No code found during the build. Please see:\n" +
       "https://docs.github.com/en/github/finding-security-vulnerabilities-and-errors-in-your-code/troubleshooting-code-scanning#no-code-found-during-the-build",
   },
+  fatalError: {
+    outputRegex: new RegExp("A fatal error occurred"),
+    message: "A fatal error occurred.",
+  },
 };
 
 // we collapse the matches into an array for use in execErrorCatcher

src/toolrunner-error-catcher.test.ts

@@ -21,7 +21,8 @@ test("matchers are never applied if non-error exit", async (t) => {
 
   t.deepEqual(await exec.exec("node", testArgs), 0);
 
-  t.deepEqual(await toolrunnerErrorCatcher("node", testArgs, matchers), 0);
+  const returnState = await toolrunnerErrorCatcher("node", testArgs, matchers);
+  t.deepEqual(returnState.exitCode, 0);
 });
 
 test("regex matchers are applied to stdout for non-zero exit code", async (t) => {
@@ -150,12 +151,11 @@ test("execErrorCatcher respects the ignoreReturnValue option", async (t) => {
     { instanceOf: Error }
   );
 
-  t.deepEqual(
-    await toolrunnerErrorCatcher("node", testArgs, [], {
-      ignoreReturnCode: true,
-    }),
-    199
-  );
+  const returnState = await toolrunnerErrorCatcher("node", testArgs, [], {
+    ignoreReturnCode: true,
+  });
+
+  t.deepEqual(returnState.exitCode, 199);
 });
 
 test("execErrorCatcher preserves behavior of provided listeners", async (t) => {
@@ -176,12 +176,10 @@ test("execErrorCatcher preserves behavior of provided listeners", async (t) => {
 
   const testArgs = buildDummyArgs(stdoutExpected, stderrExpected, "", 0);
 
-  t.deepEqual(
-    await toolrunnerErrorCatcher("node", testArgs, [], {
-      listeners,
-    }),
-    0
-  );
+  const returnState = await toolrunnerErrorCatcher("node", testArgs, [], {
+    listeners,
+  });
+  t.deepEqual(returnState.exitCode, 0);
 
   t.deepEqual(stdoutActual, `${stdoutExpected}\n`);
   t.deepEqual(stderrActual, `${stderrExpected}\n`);

src/toolrunner-error-catcher.ts

@@ -4,6 +4,11 @@ import * as safeWhich from "@chrisgavin/safe-which";
 
 import { ErrorMatcher } from "./error-matcher";
 
+export interface ReturnState {
+  exitCode: number;
+  stdout: string;
+}
+
 /**
  * Wrapper for toolrunner.Toolrunner which checks for specific return code and/or regex matches in console output.
  * Output will be streamed to the live console as well as captured for subsequent processing.
@@ -13,14 +18,14 @@ import { ErrorMatcher } from "./error-matcher";
  * @param     args               optional arguments for tool. Escaping is handled by the lib.
  * @param     matchers           defines specific codes and/or regexes that should lead to return of a custom error
  * @param     options            optional exec options.  See ExecOptions
- * @returns   Promise<number>    exit code
+ * @returns   ReturnState        exit code and stdout output, if applicable
  */
 export async function toolrunnerErrorCatcher(
   commandLine: string,
   args?: string[],
   matchers?: ErrorMatcher[],
   options?: im.ExecOptions
-): Promise<number> {
+): Promise<ReturnState> {
   let stdout = "";
   let stderr = "";
 
@@ -40,9 +45,9 @@ export async function toolrunnerErrorCatcher(
   };
 
   // we capture the original return code or error so that if no match is found we can duplicate the behavior
-  let returnState: Error | number;
+  let exitCode: number;
   try {
-    returnState = await new toolrunner.ToolRunner(
+    exitCode = await new toolrunner.ToolRunner(
       await safeWhich.safeWhich(commandLine),
       args,
       {
@@ -51,35 +56,32 @@ export async function toolrunnerErrorCatcher(
         ignoreReturnCode: true, // so we can check for specific codes using the matchers
       }
     ).exec();
-  } catch (e) {
-    returnState = e instanceof Error ? e : new Error(String(e));
-  }
 
-  // if there is a zero return code then we do not apply the matchers
-  if (returnState === 0) return returnState;
+    // if there is a zero return code then we do not apply the matchers
+    if (exitCode === 0) return { exitCode, stdout };
 
-  if (matchers) {
-    for (const matcher of matchers) {
-      if (
-        matcher.exitCode === returnState ||
-        matcher.outputRegex?.test(stderr) ||
-        matcher.outputRegex?.test(stdout)
-      ) {
-        throw new Error(matcher.message);
+    if (matchers) {
+      for (const matcher of matchers) {
+        if (
+          matcher.exitCode === exitCode ||
+          matcher.outputRegex?.test(stderr) ||
+          matcher.outputRegex?.test(stdout)
+        ) {
+          throw new Error(matcher.message);
+        }
       }
     }
-  }
 
-  if (typeof returnState === "number") {
     // only if we were instructed to ignore the return code do we ever return it non-zero
     if (options?.ignoreReturnCode) {
-      return returnState;
+      return { exitCode, stdout };
     } else {
       throw new Error(
-        `The process '${commandLine}' failed with exit code ${returnState}`
+        `The process '${commandLine}' failed with exit code ${exitCode}`
       );
     }
-  } else {
-    throw returnState;
+  } catch (e) {
+    const error = e instanceof Error ? e : new Error(String(e));
+    throw error;
   }
 }