Skip to content

Persist CodeQL version output to file rather than environment #4081

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mario-campos wants to merge 3 commits into
base: main
Choose a base branch
from
+112 −54
Open

Persist CodeQL version output to file rather than environment #4081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5f8c44b
Persist CodeQL version output to file rather than environment
mario-campos Aug 7, 2026
9183a7b
Handle file-read errors as cache misses
mario-campos Aug 10, 2026
208a88a
Simplify JSDoc of `getCachedCodeQlVersion`
mario-campos Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions lib/entry-points.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions src/environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,6 @@ export enum EnvVar {
*/
CODE_SCANNING_REF = "CODE_SCANNING_REF",

/**
* `PersistedVersionInfo` for the CodeQL CLI, so later Actions steps can reuse it instead of
* invoking `codeql version` again.
*/
CODEQL_VERSION_INFO = "CODEQL_ACTION_CLI_VERSION_INFO",

/** Whether the CodeQL Action has invoked the Go autobuilder. */
DID_AUTOBUILD_GOLANG = "CODEQL_ACTION_DID_AUTOBUILD_GOLANG",

Expand Down
96 changes: 62 additions & 34 deletions src/util.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import * as sinon from "sinon";
import * as api from "./api-client";
import { EnvVar } from "./environment";
import { getRunnerLogger } from "./logging";
import { setupTests } from "./testing-utils";
import { getTestEnv, setupTests } from "./testing-utils";
import * as util from "./util";

setupTests(test);
Expand Down Expand Up @@ -535,55 +535,83 @@ test("Failure.orElse returns the default value for a failure result", (t) => {

test.serial(

@mbg mbg Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test, and others, probably don't need to be serial anymore since process.env is no longer mutated.

@mario-campos mario-campos Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought, but Copilot assured me it's still necessary:

Yes, keep .serial() for this one.

Even though this test itself only reads malformed cache entries, it exercises getCachedCodeQlVersion, which depends on the module-global cachedCodeQlVersion state in util.ts. Other nearby tests in the same file can populate that cache, and parallel execution can interleave despite beforeEach resets, since they all share one process/module instance.

So .serial() is the safe choice unless you fully refactor these cache-state tests to avoid shared global state.

@mbg mbg Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point about cachedCodeQlVersion is fair. That said, I wouldn't be opposed to refactoring that away so that the state is threaded to these functions rather than global as part of this PR.

"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",
async (t) => {
await util.withTmpDir(async (tmpDir: string) => {
const cacheFile = path.join(tmpDir, "version.json");
fs.writeFileSync(
cacheFile,
JSON.stringify({
cmd: "/path/to/codeql",
version: { version: "2.20.0" },
}),
"utf8",
);
const env = getTestEnv({ [EnvVar.TEMP]: tmpDir });
t.deepEqual(util.getCachedCodeQlVersion("/path/to/codeql", env), {
version: "2.20.0",
});
});
},
);

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" },
async (t) => {
await util.withTmpDir(async (tmpDir: string) => {
const cacheFile = path.join(tmpDir, "version.json");
fs.writeFileSync(
cacheFile,
JSON.stringify({
cmd: "/path/to/other-codeql",
version: { version: "2.20.0" },
}),
"utf8",
);
const env = getTestEnv({ [EnvVar.TEMP]: tmpDir });
t.is(util.getCachedCodeQlVersion("/path/to/codeql", env), undefined);
});
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);
async (t) => {
await util.withTmpDir(async (tmpDir: string) => {
const cacheFile = path.join(tmpDir, "version.json");
fs.writeFileSync(cacheFile, "not valid json", "utf8");
const env = getTestEnv({ [EnvVar.TEMP]: tmpDir });
t.is(util.getCachedCodeQlVersion("/path/to/codeql", env), 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);
}
async (t) => {
await util.withTmpDir(async (tmpDir: string) => {
const cacheFile = path.join(tmpDir, "version.json");
const env = getTestEnv({ [EnvVar.TEMP]: tmpDir });
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" },
}),
]) {
Comment thread
mario-campos marked this conversation as resolved.
fs.writeFileSync(cacheFile, value, "utf8");
t.is(
util.getCachedCodeQlVersion("/path/to/codeql", env),
undefined,
value,
);
}
});
},
);
44 changes: 37 additions & 7 deletions src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ import getFolderSize from "get-folder-size";
import * as yaml from "js-yaml";
import * as semver from "semver";

import { getTemporaryDirectory } from "./actions-util";
import * as apiCompatibility from "./api-compatibility.json";
import type { CodeQL, VersionInfo } from "./codeql";
import type { Pack } from "./config/db-config";
import type { Config } from "./config-utils";
import { EnvVar, getRequiredEnvParam } from "./environment";
import { Env, EnvVar, getEnv, getRequiredEnvParam } from "./environment";
import * as json from "./json";
import { Language } from "./languages";
import { Logger } from "./logging";
Expand Down Expand Up @@ -638,7 +639,25 @@ function isPersistedVersionInfo(x: unknown): x is PersistedVersionInfo {
);
}

export function cacheCodeQlVersion(cmd: string, version: VersionInfo): void {
/**
* Returns the file path to the `codeql version` output cache.
* @param env The environment variables to use—only necessary for testing.
Comment thread
mario-campos marked this conversation as resolved.
*/
function getPathToCodeQLVersionCacheFile(env: Env): string {
return path.join(getTemporaryDirectory(env), "version.json");
}
Comment on lines +646 to +648

@mbg mbg Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to have a separate file for each command? If so, why?

@mario-campos mario-campos Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, with this PR, I'm simply trying to do a direct environment-variable-to-file translation. In subsequent PRs, I plan to generalize the file to include additional commands. The only reason that the file's name is associated with the command is to keep this PR focused on this one change, without scope creep of future intentions.


/**
* Caches the CodeQL CLI version both in-memory and on disk.
* @param cmd The path to the CodeQL CLI.
* @param version The version information to cache.
* @param env The environment variables to use—only necessary for testing.
*/
export function cacheCodeQlVersion(
cmd: string,
version: VersionInfo,
env: Env = getEnv(),

@mbg mbg Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid the default getEnv() which could lead to unexpected results.

): void {
if (cachedCodeQlVersion !== undefined) {
throw new Error("cacheCodeQlVersion() should be called only once");
}
Expand All @@ -647,21 +666,32 @@ export function cacheCodeQlVersion(cmd: string, version: VersionInfo): void {
// processes, can reuse it rather than invoking `codeql version` again. We
// record the CLI path so that a different step using a different CodeQL bundle
// doesn't pick up a stale version.
core.exportVariable(
EnvVar.CODEQL_VERSION_INFO,
fs.writeFileSync(
getPathToCodeQLVersionCacheFile(env),
JSON.stringify({ cmd, version }),
"utf8",
);
}

export function getCachedCodeQlVersion(cmd?: string): undefined | VersionInfo {
/**
* Returns the cached CodeQL CLI version, if any.
* @param cmd The path to the CodeQL CLI.
* @param env The environment variables to use—only necessary for testing.
*/
export function getCachedCodeQlVersion(
cmd?: string,
env: Env = getEnv(),

@mbg mbg Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid the default getEnv() which could lead to unexpected results.

): undefined | VersionInfo {
if (cachedCodeQlVersion !== undefined) {
return cachedCodeQlVersion;
}
// Fall back to the value persisted by an earlier Actions step, if any. This is
// best-effort: any malformed or mismatched value is ignored so that the caller
// invokes `codeql version` instead.
const serialized = process.env[EnvVar.CODEQL_VERSION_INFO];
if (!serialized) {
let serialized: string;
try {
serialized = fs.readFileSync(getPathToCodeQLVersionCacheFile(env), "utf8");
} catch {

@mbg mbg Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error here is swallowed, which might make it hard to troubleshoot issues. Consider logging it at debug-level.

return undefined;
}
let persisted: unknown;
Expand Down
Loading