Include diagnostics in failed SARIF if they were created before a database was initialised by mbg · Pull Request #4223 · github/codeql-action

mbg · 2026-10-09T22:30:12Z

Diagnostics are typically written to the database cluster and are then included in the SARIF that is generated by the CLI. However, in the CodeQL Action we may produce diagnostics before the database cluster is initialised. To work around this, we have so far relied on tracking diagnostics in memory that were created before the database cluster is ready. After the cluster is initialised, those diagnostics are then flushed to disk. However, this represents a problem if the database cluster is never successfully initialised.

This PR changes this so that diagnostics are written directly to a temporary directory on disk if the database cluster is not yet ready. They are then moved to the database cluster once it is initialised. If the cluster is never initialised, then we point the CLI at the temporary directory on disk using a --diagnostic-dir argument. The CLI will then pick up any diagnostics that are still in that location and include them in the failed SARIF.

Risk assessment

For internal use only. Please select the risk level of this change:

Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Workflow types:

Advanced setup - Impacts users who have custom CodeQL workflows.
Managed - Impacts users with dynamic workflows (Default Setup, Code Quality, ...).

Products:

Code Scanning - The changes impact analyses when analysis-kinds: code-scanning.
Code Quality - The changes impact analyses when analysis-kinds: code-quality.
Other first-party - The changes impact other first-party analyses.

Environments:

Dotcom - Impacts CodeQL workflows on github.com and/or GitHub Enterprise Cloud with Data Residency.
GHES - Impacts CodeQL workflows on GitHub Enterprise Server.

How did/will you validate this change?

Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.

How will you know if something goes wrong after this change is released?

Telemetry - I rely on existing telemetry or have made changes to the telemetry.
- Dashboards - I will watch relevant dashboards for issues after the release. Consider whether this requires this change to be released at a particular time rather than as part of a regular release.
- Alerts - New or existing monitors will trip if something goes wrong with this change.

Are there any special considerations for merging or releasing this change?

No special considerations - This change can be merged at any time.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Consider adding a changelog entry for this change.
Confirm the readme and docs have been updated if necessary.

Copilot

Warning

Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

🟡 Changes recommended

Temporary-write failures can abort initialization, cross-filesystem moves can lose diagnostics, and the new check does not validate failed SARIF export.

5 open findings

Test does not verify failed-SARIF diagnostic export · New
Best-effort diagnostics can make init fail on filesystem errors · New
Cross-filesystem diagnostic moves lack an EXDEV fallback · New
Spelling correction · New
Documentation typo · New

What changed in this PR

Persists pre-database diagnostics on disk so they can be included in failed SARIF output.

Changes:

Replaces in-memory buffering with temporary diagnostic files.
Passes temporary diagnostics to CodeQL export.
Adds unit and PR-check coverage.

File	Description
`src/diagnostics.ts`	Implements temporary persistence and database relocation.
`src/init-action.ts`	Flushes and logs temporary diagnostics.
`src/init-action-post-helper.ts`	Supplies temporary diagnostics during failed SARIF generation.
`src/codeql.ts`	Adds `--diagnostic-dir` support.
`src/diagnostics.test.ts`	Tests temporary persistence and flushing.
`src/setup-codeql.test.ts`	Provides temporary-directory test setup.
`src/config/db-config.test.ts`	Provides temporary-directory test setup.
`pr-checks/diagnostics-without-db.ts`	Checks for the expected diagnostic file.
`pr-checks/checks/diagnostics-without-db.yml`	Defines the new PR check.
`lib/entry-points.js`	Generated bundle; excluded from review.
`.github/workflows/__diagnostics-without-db.yml`	Generated workflow; excluded from review.

Files excluded by content exclusion policy (2)

.github/workflows/__diagnostics-without-db.yml
lib/entry-points.js

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Warning

Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

🟢 Approval recommended

No blocking issues remain in the reviewable changes, which include targeted unit and end-to-end coverage.

1 open finding

Keep configuration lookup lazy for single-file SARIF uploads

Files excluded by content exclusion policy (2)

.github/workflows/__diagnostics-without-db.yml
lib/entry-points.js

🧠 Review effort: Balanced

Add limited test coverage

…stic`

henrymercer · 2026-10-20T18:04:49Z

+  if (language !== undefined) {
+    return path.resolve(basePath, language, "diagnostic", "codeql-action");
+  }
+  return basePath;


If the language is undefined, then I think we still want to save this within ${basePath}/diagnostic/codeql-action.

We don't hit this now since we arbitrarily choose the first language, but I think this is worth addressing to avoid surprises if we change this behaviour.

We do hit it in the addNoLanguageDiagnostic case where config === undefined. I'll change it.

Ah, I meant to say that if we're saving it to a DB we will only ever save it under the ${language}/diagnostic/codeql-action directory.

I think this has introduced a new issue: language-specific diagnostics which are created after the config is initialized, but before the DB is created, now don't get picked up because they live in ${tempDiagnosticsRootDir}/${language}/diagnostic/codeql-action, but init-post passes --diagnostic-dir=${tempDiagnosticsRootDir}/diagnostic/codeql-action.

We could modify init post to pass --diagnostic-dir=${tempDiagnosticsRootDir}. It might also be good to add a regression test.

…d-sarif

And emit a warning if it doesn't

Copilot

🟢 Approval recommended

The focused diagnostic-persistence changes have unit and integration coverage, with no unresolved blocking findings.

1 open issue

Keep configuration lookup lazy for single-file SARIF uploads

2 files not reviewed

.github/workflows/__diagnostics-without-db.yml: Excluded by your content exclusion settings
lib/entry-points.js: Excluded by your content exclusion settings

_{Review effort: Balanced}

henrymercer

Thanks for addressing my comments, just a couple of final things before we merge this.

henrymercer · 2026-10-21T17:08:54Z

+/** Report if there are temporary diagnostics and write them to the log. */
+export function logTemporaryDiagnostics(action: ActionState<["Logger"]>) {
+  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.`,
    );


I think we can update this comment since with this PR these diagnostics should now appear on the tool status page.

henrymercer · 2026-10-21T17:11:11Z

+      if (!fs.existsSync(configPath)) {
+        logger.warning(
+          [
+            `Expected a configuration file at '${configPath}', which does not exist.`,
+            "Continuing with diagnostics export anyway, without a configuration file.",
+          ].join(" "),
+        );
+        configPath = undefined;
+      }


I think it would be good to avoid warning whenever init fails before it generated the code scanning config. Perhaps we could only warn if a database has been created?

henrymercer · 2026-10-21T17:13:14Z

+  if (language !== undefined) {
+    return path.resolve(basePath, language, "diagnostic", "codeql-action");
+  }
+  return basePath;


I think this has introduced a new issue: language-specific diagnostics which are created after the config is initialized, but before the DB is created, now don't get picked up because they live in ${tempDiagnosticsRootDir}/${language}/diagnostic/codeql-action, but init-post passes --diagnostic-dir=${tempDiagnosticsRootDir}/diagnostic/codeql-action.

We could modify init post to pass --diagnostic-dir=${tempDiagnosticsRootDir}. It might also be good to add a regression test.

mbg changed the base branch from main to mbg/fs-abstraction October 9, 2026 22:30

mbg added this pull request to stack #4224 October 9, 2026 22:30

github-actions Bot added the size/L May be hard to review label Oct 9, 2026

mbg force-pushed the mbg/diagnostics/failed-sarif branch 2 times, most recently from 520eff7 to 2613b9c Compare October 9, 2026 22:51

github-actions Bot added size/XL May be very hard to review and removed size/L May be hard to review labels Oct 12, 2026

mbg removed this pull request from stack #4224 October 12, 2026 11:00

mbg mentioned this pull request Oct 12, 2026

Store and restore "failed" configuration if init step aborts #4225

Merged

mbg force-pushed the mbg/diagnostics/failed-sarif branch from b5ce898 to 9312ede Compare October 12, 2026 11:54

mbg changed the base branch from mbg/fs-abstraction to mbg/reduce-config-requirements October 12, 2026 11:54

github-actions Bot added size/L May be hard to review and removed size/XL May be very hard to review labels Oct 12, 2026

mbg force-pushed the mbg/diagnostics/failed-sarif branch from 9312ede to 65d8aa4 Compare October 12, 2026 12:02

mbg force-pushed the mbg/diagnostics/failed-sarif branch from 65d8aa4 to 9eeab46 Compare October 12, 2026 13:23

mbg marked this pull request as ready for review October 12, 2026 13:24

mbg requested a review from a team as a code owner October 12, 2026 13:24

Copilot AI balanced review requested due to automatic review settings October 12, 2026 13:24

Copilot started reviewing on behalf of mbg October 12, 2026 13:25

View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Comment thread pr-checks/checks/diagnostics-without-db.yml Outdated

Comment thread src/diagnostics.ts Outdated

Comment thread src/diagnostics.ts

Comment thread pr-checks/checks/diagnostics-without-db.yml Outdated

Comment thread src/diagnostics.ts Outdated

mbg added this pull request to stack #4227 October 12, 2026 13:49

mbg force-pushed the mbg/diagnostics/failed-sarif branch from 4b32487 to 9104ff3 Compare October 12, 2026 16:51

github-actions Bot added size/XL May be very hard to review and removed size/L May be hard to review labels Oct 12, 2026

Base automatically changed from mbg/reduce-config-requirements to main October 13, 2026 09:46

mbg force-pushed the mbg/diagnostics/failed-sarif branch 3 times, most recently from 3cf39b0 to b5557b8 Compare October 16, 2026 11:23

mbg removed this pull request from stack #4227 October 16, 2026 16:16

mbg force-pushed the mbg/diagnostics/failed-sarif branch from 12b2055 to f69da90 Compare October 16, 2026 16:19

mbg changed the base branch from main to mbg/upload-improvements-for-4223 October 16, 2026 16:20

mbg added this pull request to stack #4250 October 16, 2026 16:20

mbg requested a balanced review from Copilot October 16, 2026 16:50

Copilot started reviewing on behalf of mbg October 16, 2026 16:50

View session

Copilot AI reviewed Oct 16, 2026

View reviewed changes

mbg force-pushed the mbg/diagnostics/failed-sarif branch from f69da90 to 840cf9d Compare October 20, 2026 16:07

Base automatically changed from mbg/upload-improvements-for-4223 to main October 20, 2026 17:48

mbg added 7 commits October 20, 2026 17:48

Refactor writeDiagnosticFile out of writeDiagnostic

56e0d42

Write diagnostics to temporary location when database isn't ready

f422d8a

Make diagnostics less dependent on global state

67f2591

Add limited test coverage

Use temporary diagnostic dir in generateFailedSarif

64723df

Refactor writeTemporaryDiagnosticFile out of `add[NoLanguage]Diagno…

581a0e1

…stic`

Omit --sarif-codescanning-config if the config file doesn't exist

ba569ac

Add e2e test

b963054

mbg force-pushed the mbg/diagnostics/failed-sarif branch from 840cf9d to b963054 Compare October 20, 2026 17:48

henrymercer reviewed Oct 20, 2026

View reviewed changes

mbg added 6 commits October 21, 2026 12:22

Use same sub-path in getDiagnosticsPath for no language case

8357829

Merge remote-tracking branch 'origin/main' into mbg/diagnostics/faile…

d7bad7d

…d-sarif

Always check whether config file exists

98b8e51

And emit a warning if it doesn't

Don't include diagnostics if FF is off

4707994

Rename env var to DIAGNOSTICS_DIR

67cd8c1

Update skipped analyze step comment

a83f6f5

mbg requested review from henrymercer and a balanced review from Copilot October 21, 2026 14:22

Copilot started reviewing on behalf of mbg October 21, 2026 14:23

View session

Clarify comment that explains how the test works

c3f2a34

Copilot AI reviewed Oct 21, 2026

View reviewed changes

henrymercer reviewed Oct 21, 2026

View reviewed changes

Conversation

mbg commented Oct 9, 2026

Risk assessment

Which use cases does this change impact?

How did/will you validate this change?

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

How will you know if something goes wrong after this change is released?

Are there any special considerations for merging or releasing this change?

Merge / deployment checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

henrymercer Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

mbg Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

henrymercer Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrymercer Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

henrymercer left a comment

Choose a reason for hiding this comment

Uh oh!

henrymercer Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

henrymercer Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

henrymercer Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

henrymercer Oct 21, 2026 •

edited

Loading