Address review comments

Author: henrymercer

lib/entry-points.js

@@ -32,6 +32,7 @@ import {
   GitHubVariant,
   GitHubVersion,
   HTTPError,
+  resetCachedCodeQlVersion,
 } from "./util";
 
 export const SAMPLE_DOTCOM_API_DETAILS = {
@@ -101,6 +102,10 @@ export function setupTests(testFn: TestFn<any>) {
     // unless the test explicitly sets one up.
     codeql.setCodeQL({});
 
+    // Reset the in-process CodeQL version cache so that it doesn't leak between
+    // tests, which each represent a separate Actions step in production.
+    resetCachedCodeQlVersion();
+
     // Replace stdout and stderr so we can record output during tests
     t.context.testOutput = "";
     const processStdoutWrite = process.stdout.write.bind(process.stdout);

src/testing-utils.ts

@@ -533,25 +533,57 @@ test("Failure.orElse returns the default value for a failure result", (t) => {
   t.is(result.orElse("default value"), "default value");
 });
 
-test("getCachedCodeQlVersion reuses a version persisted by an earlier step", (t) => {
-  process.env[EnvVar.CODEQL_VERSION_INFO] = JSON.stringify({
-    cmd: "/path/to/codeql",
-    version: { version: "2.20.0" },
-  });
-  t.deepEqual(util.getCachedCodeQlVersion("/path/to/codeql"), {
-    version: "2.20.0",
-  });
-});
+test.serial(
+  "getCachedCodeQlVersion reuses a version persisted by an earlier step",
+  (t) => {
+    process.env[EnvVar.CODEQL_VERSION_INFO] = JSON.stringify({
+      cmd: "/path/to/codeql",
+      version: { version: "2.20.0" },
+    });
+    t.deepEqual(util.getCachedCodeQlVersion("/path/to/codeql"), {
+      version: "2.20.0",
+    });
+  },
+);
 
-test("getCachedCodeQlVersion ignores a persisted version from a different CLI", (t) => {
-  process.env[EnvVar.CODEQL_VERSION_INFO] = JSON.stringify({
-    cmd: "/path/to/other-codeql",
-    version: { version: "2.20.0" },
-  });
-  t.is(util.getCachedCodeQlVersion("/path/to/codeql"), undefined);
-});
+test.serial(
+  "getCachedCodeQlVersion ignores a persisted version from a different CLI",
+  (t) => {
+    process.env[EnvVar.CODEQL_VERSION_INFO] = JSON.stringify({
+      cmd: "/path/to/other-codeql",
+      version: { version: "2.20.0" },
+    });
+    t.is(util.getCachedCodeQlVersion("/path/to/codeql"), undefined);
+  },
+);
 
-test("getCachedCodeQlVersion ignores a malformed persisted value", (t) => {
-  process.env[EnvVar.CODEQL_VERSION_INFO] = "not valid json";
-  t.is(util.getCachedCodeQlVersion("/path/to/codeql"), undefined);
-});
+test.serial(
+  "getCachedCodeQlVersion ignores a malformed persisted value",
+  (t) => {
+    process.env[EnvVar.CODEQL_VERSION_INFO] = "not valid json";
+    t.is(util.getCachedCodeQlVersion("/path/to/codeql"), undefined);
+  },
+);
+
+test.serial(
+  "getCachedCodeQlVersion ignores a persisted value with the wrong structure",
+  (t) => {
+    for (const value of [
+      JSON.stringify({ cmd: "/path/to/codeql" }),
+      JSON.stringify({ cmd: "/path/to/codeql", version: {} }),
+      JSON.stringify({ cmd: "/path/to/codeql", version: { version: 2 } }),
+      JSON.stringify({ version: { version: "2.20.0" } }),
+      JSON.stringify({
+        cmd: "/path/to/codeql",
+        version: { version: "2.20.0", overlayVersion: "1" },
+      }),
+      JSON.stringify({
+        cmd: "/path/to/codeql",
+        version: { version: "2.20.0", features: "nope" },
+      }),
+    ]) {
+      process.env[EnvVar.CODEQL_VERSION_INFO] = value;
+      t.is(util.getCachedCodeQlVersion("/path/to/codeql"), undefined, value);
+    }
+  },
+);

src/util.test.ts

@@ -619,12 +619,44 @@ export function asHTTPError(arg: any): HTTPError | undefined {
 
 let cachedCodeQlVersion: undefined | VersionInfo = undefined;
 
+/**
+ * Resets the in-process cache of the CodeQL CLI version. Only for use in tests,
+ * which exercise multiple "steps" within a single process.
+ */
+export function resetCachedCodeQlVersion(): void {
+  cachedCodeQlVersion = undefined;
+}
+
 /** The persisted version together with the CLI path it was obtained from. */
 interface PersistedVersionInfo {
   cmd: string;
   version: VersionInfo;
 }
 
+function isVersionInfo(x: unknown): x is VersionInfo {
+  const candidate = x as Partial<VersionInfo> | null;
+  return (
+    typeof candidate === "object" &&
+    candidate !== null &&
+    typeof candidate.version === "string" &&
+    (candidate.features === undefined ||
+      (typeof candidate.features === "object" &&
+        candidate.features !== null)) &&
+    (candidate.overlayVersion === undefined ||
+      typeof candidate.overlayVersion === "number")
+  );
+}
+
+function isPersistedVersionInfo(x: unknown): x is PersistedVersionInfo {
+  const candidate = x as Partial<PersistedVersionInfo> | null;
+  return (
+    typeof candidate === "object" &&
+    candidate !== null &&
+    typeof candidate.cmd === "string" &&
+    isVersionInfo(candidate.version)
+  );
+}
+
 export function cacheCodeQlVersion(cmd: string, version: VersionInfo): void {
   if (cachedCodeQlVersion !== undefined) {
     throw new Error("cacheCodeQlVersion() should be called only once");
@@ -651,19 +683,22 @@ export function getCachedCodeQlVersion(cmd?: string): undefined | VersionInfo {
   if (!serialized) {
     return undefined;
   }
-  let persisted: PersistedVersionInfo;
+  let persisted: unknown;
   try {
-    persisted = JSON.parse(serialized) as PersistedVersionInfo;
+    persisted = JSON.parse(serialized);
   } catch {
     return undefined;
   }
   if (
-    typeof persisted?.version?.version !== "string" ||
+    !isPersistedVersionInfo(persisted) ||
     (cmd !== undefined && persisted.cmd !== cmd)
   ) {
     return undefined;
   }
-  return persisted.version;
+  // Memoize the parsed value so that subsequent calls in this process don't
+  // re-parse the environment variable.
+  cachedCodeQlVersion = persisted.version;
+  return cachedCodeQlVersion;
 }
 
 export async function codeQlVersionAtLeast(