From 07c0bb294663007840028b2cb994dcbbad8e0801 Mon Sep 17 00:00:00 2001 From: "Michael B. Gale" Date: Fri, 9 Oct 2026 18:56:55 +0100 Subject: [PATCH 1/7] Refactor `writeDiagnosticFile` out of `writeDiagnostic` --- lib/entry-points.js | 25 +++++++++++++----------- src/diagnostics.ts | 46 +++++++++++++++++++++++++++------------------ 2 files changed, 42 insertions(+), 29 deletions(-) diff --git a/lib/entry-points.js b/lib/entry-points.js index e1e1dfc664..2a798beb47 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -173006,6 +173006,19 @@ function addNoLanguageDiagnostic(config, diagnostic) { unwrittenDefaultLanguageDiagnostics.push(diagnostic); } } +function writeDiagnosticFile(diagnosticsPath, diagnostic) { + (0, import_fs.mkdirSync)(diagnosticsPath, { recursive: true }); + const uniqueSuffix = (diagnosticCounter++).toString(); + const sanitizedTimestamp = diagnostic.timestamp.replace( + /[^a-zA-Z0-9.-]/g, + "" + ); + const jsonPath = import_path2.default.resolve( + diagnosticsPath, + `codeql-action-${sanitizedTimestamp}-${uniqueSuffix}.json` + ); + (0, import_fs.writeFileSync)(jsonPath, JSON.stringify(diagnostic)); +} function writeDiagnostic(config, language, diagnostic) { const logger2 = getActionsLogger(); const databasePath = language ? getCodeQLDatabasePath(config, language) : config.dbLocation; @@ -173015,17 +173028,7 @@ function writeDiagnostic(config, language, diagnostic) { "codeql-action" ); try { - (0, import_fs.mkdirSync)(diagnosticsPath, { recursive: true }); - const uniqueSuffix = (diagnosticCounter++).toString(); - const sanitizedTimestamp = diagnostic.timestamp.replace( - /[^a-zA-Z0-9.-]/g, - "" - ); - const jsonPath = import_path2.default.resolve( - diagnosticsPath, - `codeql-action-${sanitizedTimestamp}-${uniqueSuffix}.json` - ); - (0, import_fs.writeFileSync)(jsonPath, JSON.stringify(diagnostic)); + writeDiagnosticFile(diagnosticsPath, diagnostic); } catch (err) { logger2.warning(`Unable to write diagnostic message to database: ${err}`); logger2.debug(JSON.stringify(diagnostic)); diff --git a/src/diagnostics.ts b/src/diagnostics.ts index fa0e87c046..caab84934e 100644 --- a/src/diagnostics.ts +++ b/src/diagnostics.ts @@ -177,6 +177,33 @@ export function addNoLanguageDiagnostic( } } +/** + * Writes {@link diagnostic} to a file in {@link diagnosticsPath}. + */ +function writeDiagnosticFile( + diagnosticsPath: string, + diagnostic: DiagnosticMessage, +) { + // Create the directory if it doesn't exist yet. + mkdirSync(diagnosticsPath, { recursive: true }); + + // Include a monotonically increasing suffix to avoid filename collisions + // between diagnostics produced within the same millisecond. + const uniqueSuffix = (diagnosticCounter++).toString(); + // We should only need to remove colons, but to be defensive, only allow a restricted set of + // characters. + const sanitizedTimestamp = diagnostic.timestamp.replace( + /[^a-zA-Z0-9.-]/g, + "", + ); + const jsonPath = path.resolve( + diagnosticsPath, + `codeql-action-${sanitizedTimestamp}-${uniqueSuffix}.json`, + ); + + writeFileSync(jsonPath, JSON.stringify(diagnostic)); +} + /** * Writes the given diagnostic to the database. * @@ -200,24 +227,7 @@ function writeDiagnostic( ); try { - // Create the directory if it doesn't exist yet. - mkdirSync(diagnosticsPath, { recursive: true }); - - // Include a monotonically increasing suffix to avoid filename collisions - // between diagnostics produced within the same millisecond. - const uniqueSuffix = (diagnosticCounter++).toString(); - // We should only need to remove colons, but to be defensive, only allow a restricted set of - // characters. - const sanitizedTimestamp = diagnostic.timestamp.replace( - /[^a-zA-Z0-9.-]/g, - "", - ); - const jsonPath = path.resolve( - diagnosticsPath, - `codeql-action-${sanitizedTimestamp}-${uniqueSuffix}.json`, - ); - - writeFileSync(jsonPath, JSON.stringify(diagnostic)); + writeDiagnosticFile(diagnosticsPath, diagnostic); } catch (err) { logger.warning(`Unable to write diagnostic message to database: ${err}`); logger.debug(JSON.stringify(diagnostic)); From 9faec68b405dd0dfd0c93de01892ef082a1483ad Mon Sep 17 00:00:00 2001 From: "Michael B. Gale" Date: Fri, 9 Oct 2026 19:29:16 +0100 Subject: [PATCH 2/7] Write diagnostics to temporary location when database isn't ready --- lib/entry-points.js | 60 +++++++++------- src/config/db-config.test.ts | 134 ++++++++++++++++++++--------------- src/diagnostics.ts | 130 ++++++++++++++++++++++----------- src/init-action.ts | 9 +-- src/setup-codeql.test.ts | 86 ++++++++++++---------- 5 files changed, 252 insertions(+), 167 deletions(-) diff --git a/lib/entry-points.js b/lib/entry-points.js index 2a798beb47..9b4c669f43 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -172971,8 +172971,7 @@ var semver5 = __toESM(require_semver2()); // src/diagnostics.ts var import_fs = require("fs"); var import_path2 = __toESM(require("path")); -var unwrittenDiagnostics = []; -var unwrittenDefaultLanguageDiagnostics = []; +var temporaryDiagnostics = []; var diagnosticCounter = 0; function makeDiagnostic(id, name, data = void 0) { return { @@ -172981,6 +172980,13 @@ function makeDiagnostic(id, name, data = void 0) { source: { ...data?.source, id, name } }; } +function getTempDiagnosticPath(tmpDir, language) { + const root = import_path2.default.join(tmpDir, "codeql-action-temp-diagnostics"); + if (language !== void 0) { + return import_path2.default.join(root, language); + } + return root; +} function addDiagnostic(config, language, diagnostic) { const logger2 = getActionsLogger(); const databasePath = language ? getCodeQLDatabasePath(config, language) : config.dbLocation; @@ -172990,7 +172996,9 @@ function addDiagnostic(config, language, diagnostic) { logger2.debug( `Writing a diagnostic for ${language}, but the database at ${databasePath} does not exist yet.` ); - unwrittenDiagnostics.push({ diagnostic, language }); + const tempDiagnosticsPath = getTempDiagnosticPath(config.tempDir, language); + const diagnosticPath = writeDiagnosticFile(tempDiagnosticsPath, diagnostic); + temporaryDiagnostics.push({ path: diagnosticPath, language }); } } function addNoLanguageDiagnostic(config, diagnostic) { @@ -173003,7 +173011,9 @@ function addNoLanguageDiagnostic(config, diagnostic) { diagnostic ); } else { - unwrittenDefaultLanguageDiagnostics.push(diagnostic); + const tempDiagnosticsPath = getTempDiagnosticPath(getTemporaryDirectory()); + const diagnosticPath = writeDiagnosticFile(tempDiagnosticsPath, diagnostic); + temporaryDiagnostics.push({ path: diagnosticPath }); } } function writeDiagnosticFile(diagnosticsPath, diagnostic) { @@ -173018,15 +173028,15 @@ function writeDiagnosticFile(diagnosticsPath, diagnostic) { `codeql-action-${sanitizedTimestamp}-${uniqueSuffix}.json` ); (0, import_fs.writeFileSync)(jsonPath, JSON.stringify(diagnostic)); + return jsonPath; +} +function getDatabaseDiagnosticsPath(config, language) { + const databasePath = language ? getCodeQLDatabasePath(config, language) : config.dbLocation; + return import_path2.default.resolve(databasePath, "diagnostic", "codeql-action"); } function writeDiagnostic(config, language, diagnostic) { const logger2 = getActionsLogger(); - const databasePath = language ? getCodeQLDatabasePath(config, language) : config.dbLocation; - const diagnosticsPath = import_path2.default.resolve( - databasePath, - "diagnostic", - "codeql-action" - ); + const diagnosticsPath = getDatabaseDiagnosticsPath(config, language); try { writeDiagnosticFile(diagnosticsPath, diagnostic); } catch (err) { @@ -173034,30 +173044,31 @@ function writeDiagnostic(config, language, diagnostic) { logger2.debug(JSON.stringify(diagnostic)); } } -function logUnwrittenDiagnostics() { +function logTemporaryDiagnostics() { const logger2 = getActionsLogger(); - const num = unwrittenDiagnostics.length; + const num = temporaryDiagnostics.length; if (num > 0) { logger2.warning( `${num} diagnostic(s) could not be written to the database and will not appear on the Tool Status Page.` ); - for (const unwritten of unwrittenDiagnostics) { - logger2.debug(JSON.stringify(unwritten.diagnostic)); + for (const temporary of temporaryDiagnostics) { + logger2.debug((0, import_fs.readFileSync)(temporary.path, "utf-8")); } } } function flushDiagnostics(config) { const logger2 = getActionsLogger(); - const diagnosticsCount = unwrittenDiagnostics.length + unwrittenDefaultLanguageDiagnostics.length; - logger2.debug(`Writing ${diagnosticsCount} diagnostic(s) to database.`); - for (const unwritten of unwrittenDiagnostics) { - writeDiagnostic(config, unwritten.language, unwritten.diagnostic); - } - for (const unwritten of unwrittenDefaultLanguageDiagnostics) { - addNoLanguageDiagnostic(config, unwritten); + const diagnosticsCount = temporaryDiagnostics.length; + logger2.debug( + `Moving ${diagnosticsCount} diagnostic(s) to their respective databases.` + ); + for (const temporary of temporaryDiagnostics) { + const directory = getDatabaseDiagnosticsPath(config, temporary.language); + const filename = import_path2.default.basename(temporary.path); + const destination = import_path2.default.join(directory, filename); + (0, import_fs.renameSync)(temporary.path, destination); } - unwrittenDiagnostics = []; - unwrittenDefaultLanguageDiagnostics = []; + temporaryDiagnostics = []; } function makeTelemetryDiagnostic(id, name, attributes, tags) { return makeDiagnostic(id, name, { @@ -186549,6 +186560,7 @@ async function run3(actionState) { if (statusReportBase !== void 0) { await sendStatusReport(statusReportBase); } + logTemporaryDiagnostics(); return; } let overlayBaseDatabaseStats; @@ -186762,7 +186774,7 @@ exec ${goBinaryPath} "$@"` ); return; } finally { - logUnwrittenDiagnostics(); + logTemporaryDiagnostics(); } await sendCompletedStatusReport2( startedAt, diff --git a/src/config/db-config.test.ts b/src/config/db-config.test.ts index 76fa918015..386556f3d3 100644 --- a/src/config/db-config.test.ts +++ b/src/config/db-config.test.ts @@ -9,8 +9,9 @@ import { LoggedMessage, makeMacro, RecordingLogger, + setupActionsVars, } from "../testing-utils"; -import { ConfigurationError, prettyPrintPack } from "../util"; +import { ConfigurationError, prettyPrintPack, withTmpDir } from "../util"; import * as dbConfig from "./db-config"; @@ -562,69 +563,84 @@ test("mergeDefaultSetupAndUserConfigs - keeps other properties from user-supplie t.deepEqual(result, configFile); }); -test("mergeDefaultSetupAndUserConfigs - ignores, but warns about, unknown keys from Default Setup", async (t) => { - const logger = new RecordingLogger(); - const configFile: dbConfig.UserConfig = { - "query-filters": [{ exclude: { a: "b" } }], - "paths-ignore": ["path"], - }; - - const result = dbConfig.mergeDefaultSetupAndUserConfigs( - logger, - { - "default-setup": { - borg: [], - org: { - unknown: "foo", - "model-packs": [], +test.serial( + "mergeDefaultSetupAndUserConfigs - ignores, but warns about, unknown keys from Default Setup", + async (t) => { + const logger = new RecordingLogger(); + const configFile: dbConfig.UserConfig = { + "query-filters": [{ exclude: { a: "b" } }], + "paths-ignore": ["path"], + }; + + await withTmpDir(async (tmpDir) => { + setupActionsVars(tmpDir, tmpDir); + const result = dbConfig.mergeDefaultSetupAndUserConfigs( + logger, + { + "default-setup": { + borg: [], + org: { + unknown: "foo", + "model-packs": [], + }, + } as unknown as dbConfig.DefaultSetupConfig, + "paths-ignore": ["other-path"], }, - } as unknown as dbConfig.DefaultSetupConfig, - "paths-ignore": ["other-path"], - }, - configFile, - ); + configFile, + ); + + t.deepEqual(result, { + ...configFile, + "default-setup": { org: { "model-packs": [] } }, + }); + + const expectedUnrecognisedKeys = [ + ".default-setup.org.unknown", + ".default-setup.borg", + ".paths-ignore", + ].join(", "); + checkExpectedLogMessages(t, logger.messages, [ + `Unrecognised keys in Default Setup configuration: ${expectedUnrecognisedKeys}`, + ]); + }); + }, +); - t.deepEqual(result, { - ...configFile, - "default-setup": { org: { "model-packs": [] } }, - }); - - const expectedUnrecognisedKeys = [ - ".default-setup.org.unknown", - ".default-setup.borg", - ".paths-ignore", - ].join(", "); - checkExpectedLogMessages(t, logger.messages, [ - `Unrecognised keys in Default Setup configuration: ${expectedUnrecognisedKeys}`, - ]); -}); +test.serial( + "mergeDefaultSetupAndUserConfigs - warns about invalid keys from Default Setup", + async (t) => { + const logger = new RecordingLogger(); + const configFile: dbConfig.UserConfig = {}; -test("mergeDefaultSetupAndUserConfigs - warns about invalid keys from Default Setup", async (t) => { - const logger = new RecordingLogger(); - const configFile: dbConfig.UserConfig = {}; + await withTmpDir(async (tmpDir) => { + setupActionsVars(tmpDir, tmpDir); - const result = dbConfig.mergeDefaultSetupAndUserConfigs( - logger, - { - "default-setup": { - org: { - "model-packs": [123], + const result = dbConfig.mergeDefaultSetupAndUserConfigs( + logger, + { + "default-setup": { + org: { + "model-packs": [123], + }, + } as unknown as dbConfig.DefaultSetupConfig, }, - } as unknown as dbConfig.DefaultSetupConfig, - }, - configFile, - ); - - t.deepEqual(result, { - ...configFile, - "default-setup": { org: { "model-packs": [123] } }, - }); - - const expectedInvalidKeys = [".default-setup.org.model-packs[0]"].join(", "); - checkExpectedLogMessages(t, logger.messages, [ - `Invalid keys in Default Setup configuration: ${expectedInvalidKeys}`, - ]); -}); + configFile, + ); + + t.deepEqual(result, { + ...configFile, + "default-setup": { org: { "model-packs": [123] } }, + }); + + const expectedInvalidKeys = [".default-setup.org.model-packs[0]"].join( + ", ", + ); + checkExpectedLogMessages(t, logger.messages, [ + `Invalid keys in Default Setup configuration: ${expectedInvalidKeys}`, + ]); + }); + }, +); /** Parses `contents` as a configuration without validating it. */ function parseUnvalidatedConfig(contents: string): dbConfig.UserConfig { diff --git a/src/diagnostics.ts b/src/diagnostics.ts index caab84934e..f08c83c420 100644 --- a/src/diagnostics.ts +++ b/src/diagnostics.ts @@ -1,6 +1,13 @@ -import { existsSync, mkdirSync, writeFileSync } from "fs"; +import { + existsSync, + mkdirSync, + readFileSync, + renameSync, + writeFileSync, +} from "fs"; import path from "path"; +import { getTemporaryDirectory } from "./actions-util"; import type { Config } from "./config-utils"; import { Language } from "./languages"; import { getActionsLogger } from "./logging"; @@ -84,22 +91,16 @@ export type DiagnosticMessage = DiagnosticMessageOptions & { source: DiagnosticSource; }; -/** Represents a diagnostic message that has not yet been written to the database. */ -interface UnwrittenDiagnostic { - /** The diagnostic message that has not yet been written. */ - diagnostic: DiagnosticMessage; - /** The language the diagnostic is for. */ - language: Language; +/** Represents a diagnostic message that has not yet been saved to the database. */ +interface TemporaryDiagnostic { + /** The path to which the diagnostic has temporarily been written to. */ + path: string; + /** The language the diagnostic is for, if any. */ + language?: Language; } -/** A list of diagnostics which have not yet been written to disk. */ -let unwrittenDiagnostics: UnwrittenDiagnostic[] = []; - -/** - * A list of diagnostics which have not yet been written to disk, - * and where the language does not matter. - */ -let unwrittenDefaultLanguageDiagnostics: DiagnosticMessage[] = []; +/** A list of diagnostics which have not yet been saved to the database. */ +let temporaryDiagnostics: TemporaryDiagnostic[] = []; /** * Counter used to generate a unique suffix for each diagnostic filename, so that @@ -128,6 +129,29 @@ export function makeDiagnostic( }; } +/** + * Gets a path relative to {@link tmpDir} where diagnostics for {@link language} + * can temporarily be stored at before the database is initialised. If {@link language} + * is `undefined`, then the common base path for all languages relative to {@link tmpDir} + * is returned. + */ +export function getTempDiagnosticPath(tmpDir: string, language?: Language) { + const root = path.join(tmpDir, "codeql-action-temp-diagnostics"); + return getDiagnosticsPath(root, language); +} + +/** + * Gets a path relative to {@link basePath} where diagnostics for {@link language} + * should be stored at. If {@link language} is `undefined`, then {@link baseBath} + * is returned instead. + */ +export function getDiagnosticsPath(basePath: string, language?: Language) { + if (language !== undefined) { + return path.resolve(basePath, language, "diagnostic", "codeql-action"); + } + return basePath; +} + /** * Adds the given diagnostic to the database. If the database does not yet exist, * the diagnostic will be written to it once it has been created. @@ -155,7 +179,12 @@ export function addDiagnostic( `Writing a diagnostic for ${language}, but the database at ${databasePath} does not exist yet.`, ); - unwrittenDiagnostics.push({ diagnostic, language }); + // Write the diagnostic to a temporary location. + const tempDiagnosticsPath = getTempDiagnosticPath(config.tempDir, language); + const diagnosticPath = writeDiagnosticFile(tempDiagnosticsPath, diagnostic); + + // Track the temporary file. + temporaryDiagnostics.push({ path: diagnosticPath, language }); } } @@ -173,7 +202,11 @@ export function addNoLanguageDiagnostic( diagnostic, ); } else { - unwrittenDefaultLanguageDiagnostics.push(diagnostic); + const tempDiagnosticsPath = getTempDiagnosticPath(getTemporaryDirectory()); + const diagnosticPath = writeDiagnosticFile(tempDiagnosticsPath, diagnostic); + + // Track the temporary file. + temporaryDiagnostics.push({ path: diagnosticPath }); } } @@ -183,7 +216,7 @@ export function addNoLanguageDiagnostic( function writeDiagnosticFile( diagnosticsPath: string, diagnostic: DiagnosticMessage, -) { +): string { // Create the directory if it doesn't exist yet. mkdirSync(diagnosticsPath, { recursive: true }); @@ -202,6 +235,16 @@ function writeDiagnosticFile( ); writeFileSync(jsonPath, JSON.stringify(diagnostic)); + + return jsonPath; +} + +/** Gets the path where diagnostics for {@link language} should be stored in the database. */ +export function getDatabaseDiagnosticsPath( + config: Config, + language: Language | undefined, +) { + return getDiagnosticsPath(config.dbLocation, language); } /** @@ -217,14 +260,7 @@ function writeDiagnostic( diagnostic: DiagnosticMessage, ) { const logger = getActionsLogger(); - const databasePath = language - ? getCodeQLDatabasePath(config, language) - : config.dbLocation; - const diagnosticsPath = path.resolve( - databasePath, - "diagnostic", - "codeql-action", - ); + const diagnosticsPath = getDatabaseDiagnosticsPath(config, language); try { writeDiagnosticFile(diagnosticsPath, diagnostic); @@ -234,39 +270,47 @@ function writeDiagnostic( } } -/** Report if there are unwritten diagnostics and write them to the log. */ -export function logUnwrittenDiagnostics() { +/** Report if there are temporary diagnostics and write them to the log. */ +export function logTemporaryDiagnostics() { const logger = getActionsLogger(); - const num = unwrittenDiagnostics.length; + const num = temporaryDiagnostics.length; + if (num > 0) { logger.warning( `${num} diagnostic(s) could not be written to the database and will not appear on the Tool Status Page.`, ); - for (const unwritten of unwrittenDiagnostics) { - logger.debug(JSON.stringify(unwritten.diagnostic)); + for (const temporary of temporaryDiagnostics) { + logger.debug(readFileSync(temporary.path, "utf-8")); } } } -/** Writes all unwritten diagnostics to disk. */ +/** Relocates all temporary diagnostics to the respective databases. */ export function flushDiagnostics(config: Config) { const logger = getActionsLogger(); - const diagnosticsCount = - unwrittenDiagnostics.length + unwrittenDefaultLanguageDiagnostics.length; - logger.debug(`Writing ${diagnosticsCount} diagnostic(s) to database.`); + const diagnosticsCount = temporaryDiagnostics.length; + logger.debug( + `Moving ${diagnosticsCount} diagnostic(s) to their respective databases.`, + ); - for (const unwritten of unwrittenDiagnostics) { - writeDiagnostic(config, unwritten.language, unwritten.diagnostic); - } - for (const unwritten of unwrittenDefaultLanguageDiagnostics) { - addNoLanguageDiagnostic(config, unwritten); + for (const temporary of temporaryDiagnostics) { + // If `temporary.language` is `undefined`, then we didn't have a `config` at the time that + // `addNoLanguageDiagnostic` was called. In that case, we arbitrarily choose the first + // configured language here like we would have done in `addNoLanguageDiagnostic`. + const directory = getDatabaseDiagnosticsPath( + config, + temporary.language ?? config.languages[0], + ); + const filename = path.basename(temporary.path); + const destination = path.join(directory, filename); + + renameSync(temporary.path, destination); } - // Reset the unwritten diagnostics arrays. - unwrittenDiagnostics = []; - unwrittenDefaultLanguageDiagnostics = []; + // Reset the temporary diagnostics arrays. + temporaryDiagnostics = []; } /** diff --git a/src/init-action.ts b/src/init-action.ts index 245957fd5b..976c68d576 100644 --- a/src/init-action.ts +++ b/src/init-action.ts @@ -35,7 +35,7 @@ import { addDiagnostic, addNoLanguageDiagnostic, flushDiagnostics, - logUnwrittenDiagnostics, + logTemporaryDiagnostics, makeDiagnostic, makeTelemetryDiagnostic, } from "./diagnostics"; @@ -471,6 +471,7 @@ async function run( if (statusReportBase !== undefined) { await sendStatusReport(statusReportBase); } + logTemporaryDiagnostics(); return; } @@ -762,8 +763,8 @@ async function run( ); } - // Write diagnostics to the database that we previously stored in memory because the database - // did not exist until now. + // Move diagnostics to the database that we previously kept in a temporary location + // because the database did not exist until now flushDiagnostics(config); // We save the config here instead of at the end of `initConfig` because we @@ -793,7 +794,7 @@ async function run( ); return; } finally { - logUnwrittenDiagnostics(); + logTemporaryDiagnostics(); } await sendCompletedStatusReport( startedAt, diff --git a/src/setup-codeql.test.ts b/src/setup-codeql.test.ts index 3fef11e63e..70a459d93b 100644 --- a/src/setup-codeql.test.ts +++ b/src/setup-codeql.test.ts @@ -1647,16 +1647,20 @@ test.serial( }, ]); - const result = await setupCodeql.getEnabledVersionsWithOverlayBaseDatabases( - overlayMatchEnabledVersions, - ["javascript"], - createFeatures([Feature.OverlayAnalysisMatchCodeqlVersion]), - getRunnerLogger(true), - ); - t.deepEqual(result, [ - { cliVersion: "2.20.1", tagName: "codeql-bundle-v2.20.1" }, - { cliVersion: "2.20.0", tagName: "codeql-bundle-v2.20.0" }, - ]); + await withTmpDir(async (tmpDir) => { + setupActionsVars(tmpDir, tmpDir); + const result = + await setupCodeql.getEnabledVersionsWithOverlayBaseDatabases( + overlayMatchEnabledVersions, + ["javascript"], + createFeatures([Feature.OverlayAnalysisMatchCodeqlVersion]), + getRunnerLogger(true), + ); + t.deepEqual(result, [ + { cliVersion: "2.20.1", tagName: "codeql-bundle-v2.20.1" }, + { cliVersion: "2.20.0", tagName: "codeql-bundle-v2.20.0" }, + ]); + }); }, ); @@ -1773,21 +1777,25 @@ test.serial( }, ]); - const result = await setupCodeql.getEnabledVersionsWithOverlayBaseDatabases( - overlayMatchEnabledVersions, - ["javascript"], - createFeatures([Feature.OverlayAnalysisMatchCodeqlVersionDryRun]), - getRunnerLogger(true), - ); - t.deepEqual( - result, - [], - "Dry-run should return an empty list so the caller falls back.", - ); - t.assert( - listStub.calledOnce, - "Dry-run should still list Actions caches to populate the diagnostic.", - ); + await withTmpDir(async (tmpDir) => { + setupActionsVars(tmpDir, tmpDir); + const result = + await setupCodeql.getEnabledVersionsWithOverlayBaseDatabases( + overlayMatchEnabledVersions, + ["javascript"], + createFeatures([Feature.OverlayAnalysisMatchCodeqlVersionDryRun]), + getRunnerLogger(true), + ); + t.deepEqual( + result, + [], + "Dry-run should return an empty list so the caller falls back.", + ); + t.assert( + listStub.calledOnce, + "Dry-run should still list Actions caches to populate the diagnostic.", + ); + }); }, ); @@ -1801,18 +1809,22 @@ test.serial( }, ]); - const result = await setupCodeql.getEnabledVersionsWithOverlayBaseDatabases( - overlayMatchEnabledVersions, - ["javascript"], - createFeatures([ - Feature.OverlayAnalysisMatchCodeqlVersion, - Feature.OverlayAnalysisMatchCodeqlVersionDryRun, - ]), - getRunnerLogger(true), - ); - t.deepEqual(result, [ - { cliVersion: "2.20.1", tagName: "codeql-bundle-v2.20.1" }, - ]); + await withTmpDir(async (tmpDir) => { + setupActionsVars(tmpDir, tmpDir); + const result = + await setupCodeql.getEnabledVersionsWithOverlayBaseDatabases( + overlayMatchEnabledVersions, + ["javascript"], + createFeatures([ + Feature.OverlayAnalysisMatchCodeqlVersion, + Feature.OverlayAnalysisMatchCodeqlVersionDryRun, + ]), + getRunnerLogger(true), + ); + t.deepEqual(result, [ + { cliVersion: "2.20.1", tagName: "codeql-bundle-v2.20.1" }, + ]); + }); }, ); From 6ce04baafb4643f16cebceb53b47a3796efb9a02 Mon Sep 17 00:00:00 2001 From: "Michael B. Gale" Date: Fri, 9 Oct 2026 19:38:43 +0100 Subject: [PATCH 3/7] Import `fs` as qualified --- lib/entry-points.js | 22 +++++++++++----------- src/diagnostics.ts | 18 ++++++------------ 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/lib/entry-points.js b/lib/entry-points.js index 9b4c669f43..124ed3fa41 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -172969,7 +172969,7 @@ var jsonschema = __toESM(require_lib2()); var semver5 = __toESM(require_semver2()); // src/diagnostics.ts -var import_fs = require("fs"); +var nodefs = __toESM(require("fs")); var import_path2 = __toESM(require("path")); var temporaryDiagnostics = []; var diagnosticCounter = 0; @@ -172990,7 +172990,7 @@ function getTempDiagnosticPath(tmpDir, language) { function addDiagnostic(config, language, diagnostic) { const logger2 = getActionsLogger(); const databasePath = language ? getCodeQLDatabasePath(config, language) : config.dbLocation; - if ((0, import_fs.existsSync)(databasePath)) { + if (nodefs.existsSync(databasePath)) { writeDiagnostic(config, language, diagnostic); } else { logger2.debug( @@ -173017,7 +173017,7 @@ function addNoLanguageDiagnostic(config, diagnostic) { } } function writeDiagnosticFile(diagnosticsPath, diagnostic) { - (0, import_fs.mkdirSync)(diagnosticsPath, { recursive: true }); + nodefs.mkdirSync(diagnosticsPath, { recursive: true }); const uniqueSuffix = (diagnosticCounter++).toString(); const sanitizedTimestamp = diagnostic.timestamp.replace( /[^a-zA-Z0-9.-]/g, @@ -173027,7 +173027,7 @@ function writeDiagnosticFile(diagnosticsPath, diagnostic) { diagnosticsPath, `codeql-action-${sanitizedTimestamp}-${uniqueSuffix}.json` ); - (0, import_fs.writeFileSync)(jsonPath, JSON.stringify(diagnostic)); + nodefs.writeFileSync(jsonPath, JSON.stringify(diagnostic)); return jsonPath; } function getDatabaseDiagnosticsPath(config, language) { @@ -173052,7 +173052,7 @@ function logTemporaryDiagnostics() { `${num} diagnostic(s) could not be written to the database and will not appear on the Tool Status Page.` ); for (const temporary of temporaryDiagnostics) { - logger2.debug((0, import_fs.readFileSync)(temporary.path, "utf-8")); + logger2.debug(nodefs.readFileSync(temporary.path, "utf-8")); } } } @@ -173066,7 +173066,7 @@ function flushDiagnostics(config) { const directory = getDatabaseDiagnosticsPath(config, temporary.language); const filename = import_path2.default.basename(temporary.path); const destination = import_path2.default.join(directory, filename); - (0, import_fs.renameSync)(temporary.path, destination); + nodefs.renameSync(temporary.path, destination); } temporaryDiagnostics = []; } @@ -180920,7 +180920,7 @@ var artifactLegacy = __toESM(require_artifact_client2()); var core15 = __toESM(require_core()); // node_modules/archiver/lib/core.js -var import_fs2 = require("fs"); +var import_fs = require("fs"); // node_modules/is-stream/index.js function isStream(stream2, { checkOpen = true } = {}) { @@ -183136,7 +183136,7 @@ var Archiver = class extends import_readable_stream2.Transform { data.sourcePath = filepath; task.data = data; this._entriesCount++; - if (data.stats && data.stats instanceof import_fs2.Stats) { + if (data.stats && data.stats instanceof import_fs.Stats) { task = this._updateQueueTaskWithStats(task, data.stats); if (task) { if (data.stats.size) { @@ -183384,7 +183384,7 @@ var Archiver = class extends import_readable_stream2.Transform { callback(); return; } - (0, import_fs2.lstat)( + (0, import_fs.lstat)( task.filepath, function(err, stats) { if (this._state.aborted) { @@ -183446,7 +183446,7 @@ var Archiver = class extends import_readable_stream2.Transform { task.data.type = "file"; task.data.sourceType = "stream"; task.source = new import_lazystream.Readable(function() { - return (0, import_fs2.createReadStream)(task.filepath); + return (0, import_fs.createReadStream)(task.filepath); }); } else if (stats.isDirectory() && this._supportsDirectory) { task.data.name = trailingSlashIt(task.data.name); @@ -183455,7 +183455,7 @@ var Archiver = class extends import_readable_stream2.Transform { task.data.sourceType = "buffer"; task.source = Buffer.concat([]); } else if (stats.isSymbolicLink() && this._supportsSymlink) { - const linkPath = (0, import_fs2.readlinkSync)(task.filepath); + const linkPath = (0, import_fs.readlinkSync)(task.filepath); const dirName = (0, import_path7.dirname)(task.filepath); task.data.type = "symlink"; task.data.linkname = (0, import_path7.relative)( diff --git a/src/diagnostics.ts b/src/diagnostics.ts index f08c83c420..a6d137edff 100644 --- a/src/diagnostics.ts +++ b/src/diagnostics.ts @@ -1,10 +1,4 @@ -import { - existsSync, - mkdirSync, - readFileSync, - renameSync, - writeFileSync, -} from "fs"; +import * as nodefs from "fs"; import path from "path"; import { getTemporaryDirectory } from "./actions-util"; @@ -172,7 +166,7 @@ export function addDiagnostic( // Check that the database exists before writing to it. If the database does not yet exist, // store the diagnostic in memory and write it later. - if (existsSync(databasePath)) { + if (nodefs.existsSync(databasePath)) { writeDiagnostic(config, language, diagnostic); } else { logger.debug( @@ -218,7 +212,7 @@ function writeDiagnosticFile( diagnostic: DiagnosticMessage, ): string { // Create the directory if it doesn't exist yet. - mkdirSync(diagnosticsPath, { recursive: true }); + nodefs.mkdirSync(diagnosticsPath, { recursive: true }); // Include a monotonically increasing suffix to avoid filename collisions // between diagnostics produced within the same millisecond. @@ -234,7 +228,7 @@ function writeDiagnosticFile( `codeql-action-${sanitizedTimestamp}-${uniqueSuffix}.json`, ); - writeFileSync(jsonPath, JSON.stringify(diagnostic)); + nodefs.writeFileSync(jsonPath, JSON.stringify(diagnostic)); return jsonPath; } @@ -281,7 +275,7 @@ export function logTemporaryDiagnostics() { ); for (const temporary of temporaryDiagnostics) { - logger.debug(readFileSync(temporary.path, "utf-8")); + logger.debug(nodefs.readFileSync(temporary.path, "utf-8")); } } } @@ -306,7 +300,7 @@ export function flushDiagnostics(config: Config) { const filename = path.basename(temporary.path); const destination = path.join(directory, filename); - renameSync(temporary.path, destination); + nodefs.renameSync(temporary.path, destination); } // Reset the temporary diagnostics arrays. From 846426ec7506f12c123660852d6ab95680ddd9b9 Mon Sep 17 00:00:00 2001 From: "Michael B. Gale" Date: Fri, 9 Oct 2026 21:59:55 +0100 Subject: [PATCH 4/7] Make diagnostics less dependent on global state Add limited test coverage --- lib/entry-points.js | 58 ++++++++++--------- src/diagnostics.test.ts | 124 ++++++++++++++++++++++++++++++++++++++++ src/diagnostics.ts | 57 +++++++++++------- src/init-action.ts | 6 +- 4 files changed, 195 insertions(+), 50 deletions(-) create mode 100644 src/diagnostics.test.ts diff --git a/lib/entry-points.js b/lib/entry-points.js index 124ed3fa41..8b815345ef 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -172987,21 +172987,24 @@ function getTempDiagnosticPath(tmpDir, language) { } return root; } -function addDiagnostic(config, language, diagnostic) { - const logger2 = getActionsLogger(); +function addDiagnostic(config, language, diagnostic, logger2 = getActionsLogger(), fs33 = nodefs) { const databasePath = language ? getCodeQLDatabasePath(config, language) : config.dbLocation; - if (nodefs.existsSync(databasePath)) { - writeDiagnostic(config, language, diagnostic); + if (fs33.existsSync(databasePath)) { + writeDiagnostic({ logger: logger2, fs: fs33 }, config, language, diagnostic); } else { logger2.debug( `Writing a diagnostic for ${language}, but the database at ${databasePath} does not exist yet.` ); const tempDiagnosticsPath = getTempDiagnosticPath(config.tempDir, language); - const diagnosticPath = writeDiagnosticFile(tempDiagnosticsPath, diagnostic); + const diagnosticPath = writeDiagnosticFile( + fs33, + tempDiagnosticsPath, + diagnostic + ); temporaryDiagnostics.push({ path: diagnosticPath, language }); } } -function addNoLanguageDiagnostic(config, diagnostic) { +function addNoLanguageDiagnostic(config, diagnostic, fs33 = nodefs) { if (config !== void 0) { addDiagnostic( config, @@ -173012,12 +173015,16 @@ function addNoLanguageDiagnostic(config, diagnostic) { ); } else { const tempDiagnosticsPath = getTempDiagnosticPath(getTemporaryDirectory()); - const diagnosticPath = writeDiagnosticFile(tempDiagnosticsPath, diagnostic); + const diagnosticPath = writeDiagnosticFile( + fs33, + tempDiagnosticsPath, + diagnostic + ); temporaryDiagnostics.push({ path: diagnosticPath }); } } -function writeDiagnosticFile(diagnosticsPath, diagnostic) { - nodefs.mkdirSync(diagnosticsPath, { recursive: true }); +function writeDiagnosticFile(fs33, diagnosticsPath, diagnostic) { + fs33.mkdirSync(diagnosticsPath, { recursive: true }); const uniqueSuffix = (diagnosticCounter++).toString(); const sanitizedTimestamp = diagnostic.timestamp.replace( /[^a-zA-Z0-9.-]/g, @@ -173027,46 +173034,45 @@ function writeDiagnosticFile(diagnosticsPath, diagnostic) { diagnosticsPath, `codeql-action-${sanitizedTimestamp}-${uniqueSuffix}.json` ); - nodefs.writeFileSync(jsonPath, JSON.stringify(diagnostic)); + fs33.writeFileSync(jsonPath, JSON.stringify(diagnostic)); return jsonPath; } function getDatabaseDiagnosticsPath(config, language) { const databasePath = language ? getCodeQLDatabasePath(config, language) : config.dbLocation; return import_path2.default.resolve(databasePath, "diagnostic", "codeql-action"); } -function writeDiagnostic(config, language, diagnostic) { - const logger2 = getActionsLogger(); +function writeDiagnostic(action, config, language, diagnostic) { const diagnosticsPath = getDatabaseDiagnosticsPath(config, language); try { - writeDiagnosticFile(diagnosticsPath, diagnostic); + writeDiagnosticFile(action.fs, diagnosticsPath, diagnostic); } catch (err) { - logger2.warning(`Unable to write diagnostic message to database: ${err}`); - logger2.debug(JSON.stringify(diagnostic)); + action.logger.warning( + `Unable to write diagnostic message to database: ${err}` + ); + action.logger.debug(JSON.stringify(diagnostic)); } } -function logTemporaryDiagnostics() { - const logger2 = getActionsLogger(); +function logTemporaryDiagnostics(action) { const num = temporaryDiagnostics.length; if (num > 0) { - logger2.warning( + action.logger.warning( `${num} diagnostic(s) could not be written to the database and will not appear on the Tool Status Page.` ); for (const temporary of temporaryDiagnostics) { - logger2.debug(nodefs.readFileSync(temporary.path, "utf-8")); + action.logger.debug(action.fs.readFileSync(temporary.path, "utf-8")); } } } -function flushDiagnostics(config) { - const logger2 = getActionsLogger(); +function flushDiagnostics(action, config) { const diagnosticsCount = temporaryDiagnostics.length; - logger2.debug( + action.logger.debug( `Moving ${diagnosticsCount} diagnostic(s) to their respective databases.` ); for (const temporary of temporaryDiagnostics) { const directory = getDatabaseDiagnosticsPath(config, temporary.language); const filename = import_path2.default.basename(temporary.path); const destination = import_path2.default.join(directory, filename); - nodefs.renameSync(temporary.path, destination); + action.fs.renameSync(temporary.path, destination); } temporaryDiagnostics = []; } @@ -186560,7 +186566,7 @@ async function run3(actionState) { if (statusReportBase !== void 0) { await sendStatusReport(statusReportBase); } - logTemporaryDiagnostics(); + logTemporaryDiagnostics(actionState); return; } let overlayBaseDatabaseStats; @@ -186750,7 +186756,7 @@ exec ${goBinaryPath} "$@"` `${existingJavaToolOptions} -Djavax.net.debug=all` ); } - flushDiagnostics(config); + flushDiagnostics(actionState, config); await saveConfig(config, logger2); core19.setOutput("codeql-path", config.codeQLCmd); core19.setOutput("codeql-version", (await codeql.getVersion()).version); @@ -186774,7 +186780,7 @@ exec ${goBinaryPath} "$@"` ); return; } finally { - logTemporaryDiagnostics(); + logTemporaryDiagnostics(actionState); } await sendCompletedStatusReport2( startedAt, diff --git a/src/diagnostics.test.ts b/src/diagnostics.test.ts new file mode 100644 index 0000000000..e721dfc529 --- /dev/null +++ b/src/diagnostics.test.ts @@ -0,0 +1,124 @@ +import * as fs from "fs"; +import path from "path"; + +import test from "ava"; +import * as sinon from "sinon"; + +import * as diagnostics from "./diagnostics"; +import type { FileSystem } from "./fs"; +import { BuiltInLanguage } from "./languages"; +import { + checkExpectedLogMessages, + createTestConfig, + initAllState, + RecordingLogger, + setupTests, +} from "./testing-utils"; + +setupTests(test); + +type TestFS = FileSystem< + "existsSync" | "mkdirSync" | "writeFileSync" | "readFileSync" | "renameSync" +>; + +function makeStubFS() { + // The functions in `fs` cannot be stubbed. Create a new, minimal object + // that we can stub. + const stubbedFS: TestFS = { + existsSync: fs.existsSync, + mkdirSync: fs.mkdirSync, + writeFileSync: fs.writeFileSync, + readFileSync: fs.readFileSync, + renameSync: fs.renameSync, + }; + + const existsSync = sinon.stub(stubbedFS, "existsSync"); + const mkdirSync = sinon.stub(stubbedFS, "mkdirSync"); + const writeFileSync = sinon.stub(stubbedFS, "writeFileSync"); + const readFileSync = sinon.stub(stubbedFS, "readFileSync"); + const renameSync = sinon.stub(stubbedFS, "renameSync"); + + return { + stubbedFS, + existsSync, + mkdirSync, + writeFileSync, + readFileSync, + renameSync, + }; +} + +const id = "codeql-action/test-diagnostic"; +const name = "Test title"; + +test("makeDiagnostic - adds expected properties", (t) => { + const diagnostic = diagnostics.makeDiagnostic(id, name, {}); + + t.truthy(diagnostic.timestamp); + t.is(diagnostic.source.id, id); + t.is(diagnostic.source.name, name); +}); + +test("addDiagnostic writes temporary diagnostics and flushDiagnostics moves them", (t) => { + const logger = new RecordingLogger(); + const { stubbedFS, existsSync, mkdirSync, writeFileSync, renameSync } = + makeStubFS(); + + let databasePath: fs.PathLike | undefined; + existsSync.callsFake((p) => { + databasePath = p; + return false; + }); + + const diagnostic = diagnostics.makeDiagnostic(id, name, {}); + t.notThrows(() => { + diagnostics.addDiagnostic( + createTestConfig({ tempDir: path.resolve("/temp/") }), + BuiltInLanguage.actions, + diagnostic, + logger, + stubbedFS as FileSystem, + ); + diagnostics.flushDiagnostics( + initAllState({ logger, fs: stubbedFS as FileSystem }), + createTestConfig({ + tempDir: path.resolve("/temp/"), + dbLocation: path.resolve("/test/database/"), + }), + ); + }); + + // addDiagnostic + t.is(existsSync.callCount, 1); + t.is(mkdirSync.callCount, 1); + t.is(writeFileSync.callCount, 1); + t.is(writeFileSync.args[0].length, 2); + t.is(writeFileSync.args[0][1], JSON.stringify(diagnostic)); + + // flushDiagnostics + t.is(renameSync.callCount, 1); + t.is(renameSync.args[0].length, 2); + + const sourcePath = renameSync.args[0][0].toString(); + const expectedSourcePathPrefix = path.resolve( + "/temp/codeql-action-temp-diagnostics/actions/diagnostic/codeql-action/", + ); + t.true( + sourcePath.startsWith(expectedSourcePathPrefix), + `'${sourcePath}' does not start with '${expectedSourcePathPrefix}`, + ); + + const destPath = renameSync.args[0][1].toString(); + const expectedDestPathPrefix = path.resolve( + "/test/database/actions/diagnostic/codeql-action/", + ); + t.true( + destPath.startsWith(expectedDestPathPrefix), + `'${destPath}' does not start with '${expectedDestPathPrefix}`, + ); + + checkExpectedLogMessages(t, logger.messages, [ + `but the database at ${databasePath} does not exist yet`, + "Moving 1 diagnostic(s) to their respective databases.", + ]); +}); diff --git a/src/diagnostics.ts b/src/diagnostics.ts index a6d137edff..3563e41f4f 100644 --- a/src/diagnostics.ts +++ b/src/diagnostics.ts @@ -1,10 +1,12 @@ import * as nodefs from "fs"; import path from "path"; +import type { ActionState } from "./action-common"; import { getTemporaryDirectory } from "./actions-util"; import type { Config } from "./config-utils"; +import type { FileSystem } from "./fs"; import { Language } from "./languages"; -import { getActionsLogger } from "./logging"; +import { getActionsLogger, type Logger } from "./logging"; import { getCodeQLDatabasePath } from "./util"; /** @@ -158,16 +160,17 @@ export function addDiagnostic( config: Config, language: Language, diagnostic: DiagnosticMessage, + logger: Logger = getActionsLogger(), + fs: FileSystem = nodefs, ) { - const logger = getActionsLogger(); const databasePath = language ? getCodeQLDatabasePath(config, language) : config.dbLocation; // Check that the database exists before writing to it. If the database does not yet exist, // store the diagnostic in memory and write it later. - if (nodefs.existsSync(databasePath)) { - writeDiagnostic(config, language, diagnostic); + if (fs.existsSync(databasePath)) { + writeDiagnostic({ logger, fs }, config, language, diagnostic); } else { logger.debug( `Writing a diagnostic for ${language}, but the database at ${databasePath} does not exist yet.`, @@ -175,7 +178,11 @@ export function addDiagnostic( // Write the diagnostic to a temporary location. const tempDiagnosticsPath = getTempDiagnosticPath(config.tempDir, language); - const diagnosticPath = writeDiagnosticFile(tempDiagnosticsPath, diagnostic); + const diagnosticPath = writeDiagnosticFile( + fs, + tempDiagnosticsPath, + diagnostic, + ); // Track the temporary file. temporaryDiagnostics.push({ path: diagnosticPath, language }); @@ -186,6 +193,7 @@ export function addDiagnostic( export function addNoLanguageDiagnostic( config: Config | undefined, diagnostic: DiagnosticMessage, + fs: FileSystem = nodefs, ) { if (config !== undefined) { addDiagnostic( @@ -197,7 +205,11 @@ export function addNoLanguageDiagnostic( ); } else { const tempDiagnosticsPath = getTempDiagnosticPath(getTemporaryDirectory()); - const diagnosticPath = writeDiagnosticFile(tempDiagnosticsPath, diagnostic); + const diagnosticPath = writeDiagnosticFile( + fs, + tempDiagnosticsPath, + diagnostic, + ); // Track the temporary file. temporaryDiagnostics.push({ path: diagnosticPath }); @@ -208,11 +220,12 @@ export function addNoLanguageDiagnostic( * Writes {@link diagnostic} to a file in {@link diagnosticsPath}. */ function writeDiagnosticFile( + fs: FileSystem<"mkdirSync" | "writeFileSync">, diagnosticsPath: string, diagnostic: DiagnosticMessage, ): string { // Create the directory if it doesn't exist yet. - nodefs.mkdirSync(diagnosticsPath, { recursive: true }); + fs.mkdirSync(diagnosticsPath, { recursive: true }); // Include a monotonically increasing suffix to avoid filename collisions // between diagnostics produced within the same millisecond. @@ -228,7 +241,7 @@ function writeDiagnosticFile( `codeql-action-${sanitizedTimestamp}-${uniqueSuffix}.json`, ); - nodefs.writeFileSync(jsonPath, JSON.stringify(diagnostic)); + fs.writeFileSync(jsonPath, JSON.stringify(diagnostic)); return jsonPath; } @@ -249,43 +262,45 @@ export function getDatabaseDiagnosticsPath( * @param diagnostic The diagnostic message to add to the database. */ function writeDiagnostic( + action: ActionState<["Logger", "FS"]>, config: Config, language: Language | undefined, diagnostic: DiagnosticMessage, ) { - const logger = getActionsLogger(); const diagnosticsPath = getDatabaseDiagnosticsPath(config, language); try { - writeDiagnosticFile(diagnosticsPath, diagnostic); + writeDiagnosticFile(action.fs, diagnosticsPath, diagnostic); } catch (err) { - logger.warning(`Unable to write diagnostic message to database: ${err}`); - logger.debug(JSON.stringify(diagnostic)); + action.logger.warning( + `Unable to write diagnostic message to database: ${err}`, + ); + action.logger.debug(JSON.stringify(diagnostic)); } } /** Report if there are temporary diagnostics and write them to the log. */ -export function logTemporaryDiagnostics() { - const logger = getActionsLogger(); +export function logTemporaryDiagnostics(action: ActionState<["Logger", "FS"]>) { const num = temporaryDiagnostics.length; if (num > 0) { - logger.warning( + action.logger.warning( `${num} diagnostic(s) could not be written to the database and will not appear on the Tool Status Page.`, ); for (const temporary of temporaryDiagnostics) { - logger.debug(nodefs.readFileSync(temporary.path, "utf-8")); + action.logger.debug(action.fs.readFileSync(temporary.path, "utf-8")); } } } /** Relocates all temporary diagnostics to the respective databases. */ -export function flushDiagnostics(config: Config) { - const logger = getActionsLogger(); - +export function flushDiagnostics( + action: ActionState<["Logger", "FS"]>, + config: Config, +) { const diagnosticsCount = temporaryDiagnostics.length; - logger.debug( + action.logger.debug( `Moving ${diagnosticsCount} diagnostic(s) to their respective databases.`, ); @@ -300,7 +315,7 @@ export function flushDiagnostics(config: Config) { const filename = path.basename(temporary.path); const destination = path.join(directory, filename); - nodefs.renameSync(temporary.path, destination); + action.fs.renameSync(temporary.path, destination); } // Reset the temporary diagnostics arrays. diff --git a/src/init-action.ts b/src/init-action.ts index 976c68d576..3ede802503 100644 --- a/src/init-action.ts +++ b/src/init-action.ts @@ -471,7 +471,7 @@ async function run( if (statusReportBase !== undefined) { await sendStatusReport(statusReportBase); } - logTemporaryDiagnostics(); + logTemporaryDiagnostics(actionState); return; } @@ -765,7 +765,7 @@ async function run( // Move diagnostics to the database that we previously kept in a temporary location // because the database did not exist until now - flushDiagnostics(config); + flushDiagnostics(actionState, config); // We save the config here instead of at the end of `initConfig` because we // may have updated the config returned from `initConfig`, e.g. to revert to @@ -794,7 +794,7 @@ async function run( ); return; } finally { - logTemporaryDiagnostics(); + logTemporaryDiagnostics(actionState); } await sendCompletedStatusReport( startedAt, From 5048f930ba1473c7a8e25ea993b4a107116cb577 Mon Sep 17 00:00:00 2001 From: "Michael B. Gale" Date: Fri, 9 Oct 2026 22:40:36 +0100 Subject: [PATCH 5/7] Use temporary diagnostic dir in `generateFailedSarif` --- lib/entry-points.js | 26 +++++++++++++++++++------- src/codeql.ts | 5 +++++ src/init-action-post-helper.ts | 15 ++++++++++++++- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/lib/entry-points.js b/lib/entry-points.js index 8b815345ef..54362e40d0 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -172982,10 +172982,13 @@ function makeDiagnostic(id, name, data = void 0) { } function getTempDiagnosticPath(tmpDir, language) { const root = import_path2.default.join(tmpDir, "codeql-action-temp-diagnostics"); + return getDiagnosticsPath(root, language); +} +function getDiagnosticsPath(basePath, language) { if (language !== void 0) { - return import_path2.default.join(root, language); + return import_path2.default.resolve(basePath, language, "diagnostic", "codeql-action"); } - return root; + return basePath; } function addDiagnostic(config, language, diagnostic, logger2 = getActionsLogger(), fs33 = nodefs) { const databasePath = language ? getCodeQLDatabasePath(config, language) : config.dbLocation; @@ -173038,8 +173041,7 @@ function writeDiagnosticFile(fs33, diagnosticsPath, diagnostic) { return jsonPath; } function getDatabaseDiagnosticsPath(config, language) { - const databasePath = language ? getCodeQLDatabasePath(config, language) : config.dbLocation; - return import_path2.default.resolve(databasePath, "diagnostic", "codeql-action"); + return getDiagnosticsPath(config.dbLocation, language); } function writeDiagnostic(action, config, language, diagnostic) { const diagnosticsPath = getDatabaseDiagnosticsPath(config, language); @@ -173069,7 +173071,10 @@ function flushDiagnostics(action, config) { `Moving ${diagnosticsCount} diagnostic(s) to their respective databases.` ); for (const temporary of temporaryDiagnostics) { - const directory = getDatabaseDiagnosticsPath(config, temporary.language); + const directory = getDatabaseDiagnosticsPath( + config, + temporary.language ?? config.languages[0] + ); const filename = import_path2.default.basename(temporary.path); const destination = import_path2.default.join(directory, filename); action.fs.renameSync(temporary.path, destination); @@ -177318,7 +177323,7 @@ async function getCodeQLForCmd(logger2, cmd, checkVersion, env = getEnv()) { } await new toolrunner3.ToolRunner(cmd, args).exec(); }, - async diagnosticsExport(sarifFile, automationDetailsId, config) { + async diagnosticsExport(sarifFile, automationDetailsId, config, diagnosticDir) { const args = [ "diagnostics", "export", @@ -177327,6 +177332,7 @@ async function getCodeQLForCmd(logger2, cmd, checkVersion, env = getEnv()) { `--sarif-codescanning-config=${getGeneratedCodeScanningConfigPath( config )}`, + ...diagnosticDir !== void 0 ? [`--diagnostic-dir=${diagnosticDir}`] : [], ...getExtraOptionsFromEnv(["diagnostics", "export"]) ]; if (automationDetailsId !== void 0) { @@ -186879,7 +186885,13 @@ async function generateFailedSarif(logger2, features, config, category, checkout sarifFile = "../codeql-failed-run.sarif"; } if (databasePath === void 0 || !await features.getValue("export_diagnostics_enabled" /* ExportDiagnosticsEnabled */, codeql)) { - await codeql.diagnosticsExport(sarifFile, category, config); + let diagnosticDir = getTempDiagnosticPath( + config.tempDir + ); + if (!fs31.existsSync(diagnosticDir)) { + diagnosticDir = void 0; + } + await codeql.diagnosticsExport(sarifFile, category, config, diagnosticDir); } else { await codeql.databaseExportDiagnostics(databasePath, sarifFile, category); } diff --git a/src/codeql.ts b/src/codeql.ts index 25f30f877e..cb9a49518f 100644 --- a/src/codeql.ts +++ b/src/codeql.ts @@ -198,6 +198,7 @@ export interface CodeQL { sarifFile: string, automationDetailsId: string | undefined, config: Config, + diagnosticDir: string | undefined, ): Promise; /** Get the location of an extractor for the specified language. */ resolveExtractor(language: Language): Promise; @@ -919,6 +920,7 @@ async function getCodeQLForCmd( sarifFile: string, automationDetailsId: string | undefined, config: Config, + diagnosticDir: string | undefined, ): Promise { const args = [ "diagnostics", @@ -928,6 +930,9 @@ async function getCodeQLForCmd( `--sarif-codescanning-config=${getGeneratedCodeScanningConfigPath( config, )}`, + ...(diagnosticDir !== undefined + ? [`--diagnostic-dir=${diagnosticDir}`] + : []), ...getExtraOptionsFromEnv(["diagnostics", "export"]), ]; if (automationDetailsId !== undefined) { diff --git a/src/init-action-post-helper.ts b/src/init-action-post-helper.ts index 0e6dae13aa..0414cb2145 100644 --- a/src/init-action-post-helper.ts +++ b/src/init-action-post-helper.ts @@ -18,6 +18,7 @@ import { sanitizeArtifactName, } from "./debug-artifacts"; import * as dependencyCaching from "./dependency-caching"; +import { getTempDiagnosticPath } from "./diagnostics"; import { EnvVar, ReadOnlyEnv } from "./environment"; import { Feature, FeatureEnablement } from "./feature-flags"; import { Logger } from "./logging"; @@ -178,7 +179,19 @@ async function generateFailedSarif( databasePath === undefined || !(await features.getValue(Feature.ExportDiagnosticsEnabled, codeql)) ) { - await codeql.diagnosticsExport(sarifFile, category, config); + // If we don't have a database path, then we may have diagnostics that were written to a + // temporary location. Get the path that they would have been written to and check whether + // it exists. If so, pass it on to the CLI. The CLI will not complain if we give it a + // path that doesn't exist, so we are just being extra defensive here. + let diagnosticDir: string | undefined = getTempDiagnosticPath( + config.tempDir, + ); + + if (!fs.existsSync(diagnosticDir)) { + diagnosticDir = undefined; + } + + await codeql.diagnosticsExport(sarifFile, category, config, diagnosticDir); } else { // We call 'database export-diagnostics' to find any per-database diagnostics. await codeql.databaseExportDiagnostics(databasePath, sarifFile, category); From ea80c51945eef8321aae2c3757f738b9a7a36628 Mon Sep 17 00:00:00 2001 From: "Michael B. Gale" Date: Fri, 9 Oct 2026 23:09:58 +0100 Subject: [PATCH 6/7] Add e2e test --- .../workflows/__diagnostics-without-db.yml | 86 +++++++++++++++++++ pr-checks/checks/diagnostics-without-db.yml | 27 ++++++ pr-checks/diagnostics-without-db.ts | 40 +++++++++ 3 files changed, 153 insertions(+) create mode 100644 .github/workflows/__diagnostics-without-db.yml create mode 100644 pr-checks/checks/diagnostics-without-db.yml create mode 100644 pr-checks/diagnostics-without-db.ts diff --git a/.github/workflows/__diagnostics-without-db.yml b/.github/workflows/__diagnostics-without-db.yml new file mode 100644 index 0000000000..9c5b64d22e --- /dev/null +++ b/.github/workflows/__diagnostics-without-db.yml @@ -0,0 +1,86 @@ +# Warning: This file is generated automatically, and should not be modified. +# Instead, please modify the template in the pr-checks directory and run: +# pr-checks/sync.sh +# to regenerate this file. + +name: PR Check - Diagnostic export without database +env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GO111MODULE: auto +on: + push: + branches: + - main + - releases/v* + pull_request: {} + merge_group: + types: + - checks_requested + schedule: + - cron: '0 5 * * *' + workflow_dispatch: + inputs: {} + workflow_call: + inputs: {} +defaults: + run: + shell: bash +concurrency: + cancel-in-progress: ${{ github.event_name == 'pull_request' || false }} + group: diagnostics-without-db-${{github.ref}} +jobs: + diagnostics-without-db: + strategy: + fail-fast: false + matrix: + include: + - os: ubuntu-latest + version: linked + name: Diagnostic export without database + if: github.triggering_actor != 'dependabot[bot]' + permissions: + contents: read + security-events: read + timeout-minutes: 45 + runs-on: ${{ matrix.os }} + steps: + # We deliberately need to make this step fail before the database is initialised, but after + # some diagnostic has been produced. The simplest way to do that right now is to force the + # use of a nightly release and then fail because we are attempting to analyse Swift on + # a non-macOS runner. + - name: Check out repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Install Node.js + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: 20.x + cache: npm + - name: Install newer npm + run: npm install -g npm@11.19.1 + - name: Install dependencies + run: npm ci + - name: Prepare test + id: prepare-test + uses: ./.github/actions/prepare-test + with: + version: ${{ matrix.version }} + use-all-platform-bundle: 'false' + setup-kotlin: 'true' + - name: Initialise failing analysis + id: init + uses: ./../action/init + continue-on-error: true + env: + CODEQL_ACTION_FORCE_NIGHTLY: true + with: + languages: swift + tools: ${{ steps.prepare-test.outputs.tools-url }} + + - name: Check diagnostics appear on disk + env: + SARIF_PATH: '${{ runner.temp }}/codeql-action-temp-diagnostics/' + run: | + cd ../action + npx tsx ./pr-checks/diagnostics-without-db.ts + env: + CODEQL_ACTION_TEST_MODE: true diff --git a/pr-checks/checks/diagnostics-without-db.yml b/pr-checks/checks/diagnostics-without-db.yml new file mode 100644 index 0000000000..603fdd04c5 --- /dev/null +++ b/pr-checks/checks/diagnostics-without-db.yml @@ -0,0 +1,27 @@ +name: "Diagnostic export without database" +description: "Tests that temporary diagnostics are correctly included in the failed SARIF." +versions: + # Overriden by the FF below. + - linked +installNode: true +steps: + # We deliberately need to make this step fail before the database is initialised, but after + # some diagnostic has been produced. The simplest way to do that right now is to force the + # use of a nightly release and then fail because we are attempting to analyse Swift on + # a non-macOS runner. + - name: Initialise failing analysis + id: init + uses: ./../action/init + continue-on-error: true + env: + CODEQL_ACTION_FORCE_NIGHTLY: true + with: + languages: swift + tools: ${{ steps.prepare-test.outputs.tools-url }} + + - name: Check diagnostics appear on disk + env: + SARIF_PATH: "${{ runner.temp }}/codeql-action-temp-diagnostics/" + run: | + cd ../action + npx tsx ./pr-checks/diagnostics-without-db.ts diff --git a/pr-checks/diagnostics-without-db.ts b/pr-checks/diagnostics-without-db.ts new file mode 100644 index 0000000000..b7e99c939e --- /dev/null +++ b/pr-checks/diagnostics-without-db.ts @@ -0,0 +1,40 @@ +import * as fs from "fs"; +import * as path from "path"; + +import * as core from "@actions/core"; + +import type * as diagnostics from "../src/diagnostics"; + +const DIAGNOSTIC_ID = "codeql-action/forced-nightly-cli"; +const sarifDirectory = process.env["SARIF_PATH"]; + +if (sarifDirectory === undefined) { + throw new Error("SARIF_PATH is undefined."); +} + +const paths = fs.readdirSync(sarifDirectory, { + recursive: true, + encoding: "utf-8", +}); + +let found = false; +for (const diagnosticPath of paths) { + if (path.extname(diagnosticPath) !== ".json") { + core.info(`Skipping ${diagnosticPath}.`); + continue; + } + core.info(`Found ${diagnosticPath}.`); + + const contents = JSON.parse( + fs.readFileSync(path.resolve(sarifDirectory, diagnosticPath), "utf-8"), + ) as diagnostics.DiagnosticMessage; + + if (contents.source.id === DIAGNOSTIC_ID) { + found = true; + break; + } +} + +if (!found) { + throw new Error(`Didn't find '${DIAGNOSTIC_ID}' diagnostic.`); +} From 2613b9ca2bc0061a584d1218ea0cbb5f9a6175fb Mon Sep 17 00:00:00 2001 From: "Michael B. Gale" Date: Fri, 9 Oct 2026 23:38:11 +0100 Subject: [PATCH 7/7] Fix `flushDiagnostics` not ensuring directories exist --- lib/entry-points.js | 13 +++++++++++-- src/diagnostics.test.ts | 3 +-- src/diagnostics.ts | 16 +++++++++++++--- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/lib/entry-points.js b/lib/entry-points.js index 54362e40d0..7376558e1d 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -173070,6 +173070,7 @@ function flushDiagnostics(action, config) { action.logger.debug( `Moving ${diagnosticsCount} diagnostic(s) to their respective databases.` ); + const failedFlushDiagnostics = []; for (const temporary of temporaryDiagnostics) { const directory = getDatabaseDiagnosticsPath( config, @@ -173077,9 +173078,17 @@ function flushDiagnostics(action, config) { ); const filename = import_path2.default.basename(temporary.path); const destination = import_path2.default.join(directory, filename); - action.fs.renameSync(temporary.path, destination); + try { + action.fs.mkdirSync(directory, { recursive: true }); + action.fs.renameSync(temporary.path, destination); + } catch (err) { + action.logger.warning( + `Failed to rename '${temporary.path}' to '${destination}': ${getErrorMessage(err)}` + ); + failedFlushDiagnostics.push(temporary); + } } - temporaryDiagnostics = []; + temporaryDiagnostics = failedFlushDiagnostics; } function makeTelemetryDiagnostic(id, name, attributes, tags) { return makeDiagnostic(id, name, { diff --git a/src/diagnostics.test.ts b/src/diagnostics.test.ts index e721dfc529..7d898defc7 100644 --- a/src/diagnostics.test.ts +++ b/src/diagnostics.test.ts @@ -88,9 +88,8 @@ test("addDiagnostic writes temporary diagnostics and flushDiagnostics moves them ); }); - // addDiagnostic t.is(existsSync.callCount, 1); - t.is(mkdirSync.callCount, 1); + t.is(mkdirSync.callCount, 2); t.is(writeFileSync.callCount, 1); t.is(writeFileSync.args[0].length, 2); t.is(writeFileSync.args[0][1], JSON.stringify(diagnostic)); diff --git a/src/diagnostics.ts b/src/diagnostics.ts index 3563e41f4f..a29ef25ec3 100644 --- a/src/diagnostics.ts +++ b/src/diagnostics.ts @@ -7,7 +7,7 @@ import type { Config } from "./config-utils"; import type { FileSystem } from "./fs"; import { Language } from "./languages"; import { getActionsLogger, type Logger } from "./logging"; -import { getCodeQLDatabasePath } from "./util"; +import { getCodeQLDatabasePath, getErrorMessage } from "./util"; /** * Known tags for diagnostics. There is currently only "internal-error", @@ -304,6 +304,7 @@ export function flushDiagnostics( `Moving ${diagnosticsCount} diagnostic(s) to their respective databases.`, ); + const failedFlushDiagnostics: TemporaryDiagnostic[] = []; for (const temporary of temporaryDiagnostics) { // If `temporary.language` is `undefined`, then we didn't have a `config` at the time that // `addNoLanguageDiagnostic` was called. In that case, we arbitrarily choose the first @@ -315,11 +316,20 @@ export function flushDiagnostics( const filename = path.basename(temporary.path); const destination = path.join(directory, filename); - action.fs.renameSync(temporary.path, destination); + try { + action.fs.mkdirSync(directory, { recursive: true }); + action.fs.renameSync(temporary.path, destination); + } catch (err) { + action.logger.warning( + `Failed to rename '${temporary.path}' to '${destination}': ${getErrorMessage(err)}`, + ); + + failedFlushDiagnostics.push(temporary); + } } // Reset the temporary diagnostics arrays. - temporaryDiagnostics = []; + temporaryDiagnostics = failedFlushDiagnostics; } /**