Merge pull request #1631 from github/henrymercer/duplicate-diagnostics-fixed-in-cli

Skip the SARIF notification object workaround for CLIs that have fixed this bug

.github/workflows/__diagnostics-export.yml

@@ -25,6 +25,12 @@ jobs:
     strategy:
       matrix:
         include:
+        - os: ubuntu-latest
+          version: stable-20230317
+        - os: macos-latest
+          version: stable-20230317
+        - os: windows-latest
+          version: stable-20230317
         - os: ubuntu-latest
           version: latest
         - os: macos-latest

lib/codeql.js

@@ -23,7 +23,7 @@ var __importStar = (this && this.__importStar) || function (mod) {
     return result;
 };
 Object.defineProperty(exports, "__esModule", { value: true });
-exports.enrichEnvironment = exports.getExtraOptions = exports.getCodeQLForCmd = exports.getCodeQLForTesting = exports.getCachedCodeQL = exports.setCodeQL = exports.getCodeQL = exports.setupCodeQL = exports.CODEQL_VERSION_INIT_WITH_QLCONFIG = exports.CODEQL_VERSION_SECURITY_EXPERIMENTAL_SUITE = exports.CODEQL_VERSION_BETTER_RESOLVE_LANGUAGES = exports.CODEQL_VERSION_ML_POWERED_QUERIES_WINDOWS = exports.CODEQL_VERSION_TRACING_GLIBC_2_34 = exports.CODEQL_VERSION_NEW_TRACING = exports.CODEQL_VERSION_GHES_PACK_DOWNLOAD = exports.CommandInvocationError = void 0;
+exports.enrichEnvironment = exports.getExtraOptions = exports.getCodeQLForCmd = exports.getCodeQLForTesting = exports.getCachedCodeQL = exports.setCodeQL = exports.getCodeQL = exports.setupCodeQL = exports.CODEQL_VERSION_DUPLICATE_NOTIFICATIONS_FIXED = exports.CODEQL_VERSION_INIT_WITH_QLCONFIG = exports.CODEQL_VERSION_SECURITY_EXPERIMENTAL_SUITE = exports.CODEQL_VERSION_BETTER_RESOLVE_LANGUAGES = exports.CODEQL_VERSION_ML_POWERED_QUERIES_WINDOWS = exports.CODEQL_VERSION_TRACING_GLIBC_2_34 = exports.CODEQL_VERSION_NEW_TRACING = exports.CODEQL_VERSION_GHES_PACK_DOWNLOAD = exports.CommandInvocationError = void 0;
 const fs = __importStar(require("fs"));
 const path = __importStar(require("path"));
 const core = __importStar(require("@actions/core"));
@@ -106,6 +106,11 @@ exports.CODEQL_VERSION_SECURITY_EXPERIMENTAL_SUITE = "2.12.1";
  * Versions 2.12.4+ of the CodeQL CLI support the `--qlconfig-file` flag in calls to `database init`.
  */
 exports.CODEQL_VERSION_INIT_WITH_QLCONFIG = "2.12.4";
+/**
+ * Versions 2.12.6+ of the CodeQL CLI fix a bug where duplicate notification objects could be produced,
+ * leading to an invalid SARIF output.
+ */
+exports.CODEQL_VERSION_DUPLICATE_NOTIFICATIONS_FIXED = "2.12.6";
 /**
  * Set up CodeQL CLI access.
  *
@@ -509,7 +514,9 @@ async function getCodeQLForCmd(cmd, checkVersion) {
         },
         async databaseInterpretResults(databasePath, querySuitePaths, sarifFile, addSnippetsFlag, threadsFlag, verbosityFlag, automationDetailsId, config, features, logger) {
             const shouldExportDiagnostics = await features.getValue(feature_flags_1.Feature.ExportDiagnosticsEnabled, this);
-            const codeqlOutputFile = shouldExportDiagnostics
+            const shouldWorkaroundInvalidNotifications = shouldExportDiagnostics &&
+                !(await util.codeQlVersionAbove(this, exports.CODEQL_VERSION_DUPLICATE_NOTIFICATIONS_FIXED));
+            const codeqlOutputFile = shouldWorkaroundInvalidNotifications
                 ? path.join(config.tempDir, "codeql-intermediate-results.sarif")
                 : sarifFile;
             const codeqlArgs = [
@@ -546,7 +553,7 @@ async function getCodeQLForCmd(cmd, checkVersion) {
             }
             // capture stdout, which contains analysis summaries
             const returnState = await (0, toolrunner_error_catcher_1.toolrunnerErrorCatcher)(cmd, codeqlArgs, error_matcher_1.errorMatchers);
-            if (shouldExportDiagnostics) {
+            if (shouldWorkaroundInvalidNotifications) {
                 util.fixInvalidNotificationsInFile(codeqlOutputFile, sarifFile, logger);
             }
             return returnState.stdout;
@@ -626,14 +633,17 @@ async function getCodeQLForCmd(cmd, checkVersion) {
             await new toolrunner.ToolRunner(cmd, args).exec();
         },
         async databaseExportDiagnostics(databasePath, sarifFile, automationDetailsId, tempDir, logger) {
-            const intermediateSarifFile = path.join(tempDir, "codeql-intermediate-results.sarif");
+            const shouldWorkaroundInvalidNotifications = !(await util.codeQlVersionAbove(this, exports.CODEQL_VERSION_DUPLICATE_NOTIFICATIONS_FIXED));
+            const codeqlOutputFile = shouldWorkaroundInvalidNotifications
+                ? path.join(tempDir, "codeql-intermediate-results.sarif")
+                : sarifFile;
             const args = [
                 "database",
                 "export-diagnostics",
                 `${databasePath}`,
                 "--db-cluster",
                 "--format=sarif-latest",
-                `--output=${intermediateSarifFile}`,
+                `--output=${codeqlOutputFile}`,
                 "--sarif-include-diagnostics",
                 "-vvv",
                 ...getExtraOptionsFromEnv(["diagnostics", "export"]),
@@ -642,8 +652,10 @@ async function getCodeQLForCmd(cmd, checkVersion) {
                 args.push("--sarif-category", automationDetailsId);
             }
             await new toolrunner.ToolRunner(cmd, args).exec();
-            // Fix invalid notifications in the SARIF file output by CodeQL.
+            if (shouldWorkaroundInvalidNotifications) {
-            util.fixInvalidNotificationsInFile(intermediateSarifFile, sarifFile, logger);
+                // Fix invalid notifications in the SARIF file output by CodeQL.
+                util.fixInvalidNotificationsInFile(codeqlOutputFile, sarifFile, logger);
+            }
         },
         async diagnosticsExport(sarifFile, automationDetailsId, config, features) {
             const args = [

lib/util.js

@@ -719,6 +719,9 @@ function fixInvalidNotifications(sarif, logger) {
         logger.info(`Removed ${numDuplicateLocationsRemoved} duplicate locations from SARIF notification ` +
             "objects.");
     }
+    else {
+        logger.debug("No duplicate locations found in SARIF notification objects.");
+    }
     return newSarif;
 }
 exports.fixInvalidNotifications = fixInvalidNotifications;

lib/util.test.js

@@ -363,7 +363,11 @@ const stubLocation = {
     const messages = [];
     const result = util.fixInvalidNotifications(createMockSarifWithNotification([stubLocation]), (0, testing_utils_1.getRecordingLogger)(messages));
     t.deepEqual(result, createMockSarifWithNotification([stubLocation]));
-    t.is(messages.length, 0);
+    t.is(messages.length, 1);
+    t.deepEqual(messages[0], {
+        type: "debug",
+        message: "No duplicate locations found in SARIF notification objects.",
+    });
 });
 (0, ava_1.default)("fixInvalidNotifications removes duplicate locations", (t) => {
     const messages = [];

pr-checks/checks/diagnostics-export.yml

@@ -1,6 +1,8 @@
 name: "Diagnostic export"
 description: "Tests that manually added diagnostics are correctly exported to SARIF."
-versions: ["latest", "nightly-latest"]
+# Test on 2.12.5 (which requires a workaround in the Action), the latest release, and the latest
+# nightly.
+versions: ["stable-20230317", "latest", "nightly-latest"]
 env:
   CODEQL_ACTION_EXPORT_DIAGNOSTICS: true
 steps:

src/codeql.ts

@@ -319,6 +319,12 @@ export const CODEQL_VERSION_SECURITY_EXPERIMENTAL_SUITE = "2.12.1";
  */
 export const CODEQL_VERSION_INIT_WITH_QLCONFIG = "2.12.4";
 
+/**
+ * Versions 2.12.6+ of the CodeQL CLI fix a bug where duplicate notification objects could be produced,
+ * leading to an invalid SARIF output.
+ */
+export const CODEQL_VERSION_DUPLICATE_NOTIFICATIONS_FIXED = "2.12.6";
+
 /**
  * Set up CodeQL CLI access.
  *
@@ -878,7 +884,13 @@ export async function getCodeQLForCmd(
         Feature.ExportDiagnosticsEnabled,
         this
       );
-      const codeqlOutputFile = shouldExportDiagnostics
+      const shouldWorkaroundInvalidNotifications =
+        shouldExportDiagnostics &&
+        !(await util.codeQlVersionAbove(
+          this,
+          CODEQL_VERSION_DUPLICATE_NOTIFICATIONS_FIXED
+        ));
+      const codeqlOutputFile = shouldWorkaroundInvalidNotifications
         ? path.join(config.tempDir, "codeql-intermediate-results.sarif")
         : sarifFile;
       const codeqlArgs = [
@@ -924,7 +936,7 @@ export async function getCodeQLForCmd(
         errorMatchers
       );
 
-      if (shouldExportDiagnostics) {
+      if (shouldWorkaroundInvalidNotifications) {
         util.fixInvalidNotificationsInFile(codeqlOutputFile, sarifFile, logger);
       }
 
@@ -1027,17 +1039,21 @@ export async function getCodeQLForCmd(
       tempDir: string,
       logger: Logger
     ): Promise<void> {
-      const intermediateSarifFile = path.join(
+      const shouldWorkaroundInvalidNotifications =
-        tempDir,
+        !(await util.codeQlVersionAbove(
-        "codeql-intermediate-results.sarif"
+          this,
-      );
+          CODEQL_VERSION_DUPLICATE_NOTIFICATIONS_FIXED
+        ));
+      const codeqlOutputFile = shouldWorkaroundInvalidNotifications
+        ? path.join(tempDir, "codeql-intermediate-results.sarif")
+        : sarifFile;
       const args = [
         "database",
         "export-diagnostics",
         `${databasePath}`,
         "--db-cluster", // Database is always a cluster for CodeQL versions that support diagnostics.
         "--format=sarif-latest",
-        `--output=${intermediateSarifFile}`,
+        `--output=${codeqlOutputFile}`,
         "--sarif-include-diagnostics", // ExportDiagnosticsEnabled is always true if this command is run.
         "-vvv",
         ...getExtraOptionsFromEnv(["diagnostics", "export"]),
@@ -1047,12 +1063,10 @@ export async function getCodeQLForCmd(
       }
       await new toolrunner.ToolRunner(cmd, args).exec();
 
-      // Fix invalid notifications in the SARIF file output by CodeQL.
+      if (shouldWorkaroundInvalidNotifications) {
-      util.fixInvalidNotificationsInFile(
+        // Fix invalid notifications in the SARIF file output by CodeQL.
-        intermediateSarifFile,
+        util.fixInvalidNotificationsInFile(codeqlOutputFile, sarifFile, logger);
-        sarifFile,
+      }
-        logger
-      );
     },
     async diagnosticsExport(
       sarifFile: string,

src/util.test.ts

@@ -441,7 +441,11 @@ test("fixInvalidNotifications leaves notifications with unique locations alone",
     getRecordingLogger(messages)
   );
   t.deepEqual(result, createMockSarifWithNotification([stubLocation]));
-  t.is(messages.length, 0);
+  t.is(messages.length, 1);
+  t.deepEqual(messages[0], {
+    type: "debug",
+    message: "No duplicate locations found in SARIF notification objects.",
+  });
 });
 
 test("fixInvalidNotifications removes duplicate locations", (t) => {

src/util.ts

@@ -875,6 +875,8 @@ export function fixInvalidNotifications(
       `Removed ${numDuplicateLocationsRemoved} duplicate locations from SARIF notification ` +
         "objects."
     );
+  } else {
+    logger.debug("No duplicate locations found in SARIF notification objects.");
   }
   return newSarif;
 }