Overlay: check query packs for compatibility #2993

cklin · 2025-07-25T21:32:59Z

This PR updates the init action to prevent overlay analysis (or overlay-base database construction) when one of the query packs involved does not support overlay analysis with the installed CodeQL CLI.

@alexet Please review the aspects concerning interaction with the CodeQL bundle
@mbg Please review all other aspects of the change

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

alexet

It doesn't feel to me that resolve packs is quite the right command (although it looks like it will work).It feels like we should run resolve queries with --format startingpacks to find the query packs we care about, and then just use those.

But I am also not fully sure of all the subtleties of either choice.

cklin · 2025-07-31T16:06:10Z

It doesn't feel to me that resolve packs is quite the right command (although it looks like it will work).It feels like we should run resolve queries with --format startingpacks to find the query packs we care about, and then just use those.

But I am also not fully sure of all the subtleties of either choice.

Thanks for the suggestion!

When I first looked into the problem, I considered this approach but was unable to get it to work. While codeql resolve queries with --format startingpacks can map queries to their containing query packs, there does not seem to be a way to get the list of queries that would be used in the analysis and feed that into codeql resolve queries. I could see that codeql database init already computes that information (though a deep-plumbing subcommand) for internal use, but no easy way to obtain that information from outside the CodeQL CLI. Re-interpreting the Code Scanning config file in the action seems like a lot of work for relatively little gain.

Prompted by your suggestion, I looked again. This time I noticed that codeql database init actually generates config-queries.qls suite file with the list of queries, so all I need to do is to feed that suite file into codeql resolve queries, and I can get the list of query packs to check for overlay compatibility.

(There is the slight complication that codeql resolve queries --format startingpacks would also return query packs that have not been compiled. I will have to make the action ignore those query packs when checking for overlay compatibility.)

cklin · 2025-07-31T20:38:00Z

It doesn't feel to me that resolve packs is quite the right command (although it looks like it will work).It feels like we should run resolve queries with --format startingpacks to find the query packs we care about, and then just use those.

I updated the PR to use this approach to identify the query packs to check for overlay compatibility. Related changes:

I removed the commit that adds resolvePacks() to the CodeQL interface
Instead, there is now a commit to add resolveQueriesStartingPacks()
To avoid confusion, I removed the unused resolveQueries() from CodeQL
While I am at it, I also removed the unused packDownload() from CodeQL

The other commits remain the same as before. PTAL.

cklin · 2025-08-05T18:09:48Z

The latest force push updated CODEQL_OVERLAY_MINIMUM_VERSION to 2.22.3 at Alex's request.

mbg

As always, thanks for taking on this work! I have added a few suggestions, questions, and points for discussion.

src/codeql.ts

mbg · 2025-08-05T17:56:07Z

src/codeql.ts

+      const output = await runCli(cmd, codeqlArgs, { noStreamStdout: true });
+
+      try {
+        return JSON.parse(output) as string[];


Not specific to this PR: I see that we follow the same JSON.parse(output) as type pattern throughout the implementation here, but that doesn't actually give us any guarantees that the result of JSON.parse is compatible with the type here. This may effectively delay an error until a later point. Where possible, we should probably check that the result actually what we expect (and not just valid JSON) as done in e.g. #2956.

I don't think this has to be addressed here, but we might want to look into improving this throughout in the future.

Acknowledged. I will leave it to you to decide if and when you want to perform that overall cleanup.

mbg · 2025-08-05T17:59:01Z

src/init.ts

+    const suitePath = path.join(
+      config.dbLocation,
+      language,
+      "temp",
+      "config-queries.qls",
+    );


I think this is now at least the second place where we construct this path (after my changes in #2935) so we may want to add a utility function that constructs this path for a given config+language somewhere.

mbg · 2025-08-05T18:11:35Z

src/init.ts

+    for (const packDir of packDirs) {
+      if (
+        !checkPackForOverlayCompatibility(packDir, codeQlOverlayVersion, logger)
+      ) {
+        return false;
+      }
+    }


Minor: perhaps this could be nicer as something like:

if (packDirs.some((packDir) => !checkPackForOverlayCompatibility(packDir, codeQlOverlayVersion, logger))) { return false; }

You could potentially also chain something together with the outer loop.

mbg · 2025-08-05T18:12:46Z

src/init.ts

+    const qlpackPath = path.join(packDir, "qlpack.yml");
+    const qlpackContents = yaml.load(
+      fs.readFileSync(qlpackPath, "utf8"),
+    ) as any;


I agree with Copilot here - it would be good to have typings for (at least) the parts of the format that are used here.

This might also make the tests a little nicer since you can then have test objects of this type that you can serialise, rather than hard-coded strings.

mbg · 2025-08-05T18:15:20Z

src/init.ts

+
+    const packInfoFileContents = JSON.parse(
+      fs.readFileSync(packInfoPath, "utf8"),
+    );


Agreed with Copilot. It would be good to distinguish errors while reading/parsing JSON from other errors.

mbg · 2025-08-05T18:16:30Z

src/init.ts

+      logger.warning(
+        `The query pack at ${packDir} is not compatible with overlay analysis.`,
+      );


Minor: This is very non-specific. How about "The query pack at ${packDir} does not contain the required overlayVersion property."?

Mentioning the overlayVersion property would make the message more specific, but it would not add any useful information.

Code Scanning users do not know what the overlayVersion property is or what it does. It is an implementation detail used to indicate presence and compatibility of overlay support. If a customer files a support ticket asking about the overlayVersion property being absent, our answer would be "that means your pack is not compatible with overlay analysis".

So I don't see how adding that detail would be useful.

I see Copilot agreed with my comment on this 😅

It might not be useful for users to know this implementation detail, but it would be helpful for us when troubleshooting issues related to this. It might be clear to you (and perhaps even me) what this error means since we know where in the code it is raised, but someone else might need to review the code to figure that out.

Just to be clear, I don't care whether you explicitly mention overlayVersion in the error or not. But it would be good to have a message that points more clearly at there being something wrong with the file (for whatever reason) than a compatibility issue for an entirely unspecified reason.

mbg · 2025-08-05T18:17:42Z

src/init.ts

+      logger.warning(
+        `The query pack at ${packDir} was compiled with ` +
+          `overlay version ${packOverlayVersion}, but the CodeQL CLI ` +
+          `supports only overlay version ${codeQlOverlayVersion}. The ` +


The wording here suggests that codeQlOverlayVersion is smaller than packOverlayVersion, but the check just tests that they are not equal. Should the condition be different or the wording of the warning?

The !== comparison is intended. If the pack overlay version is lower than the CLI overlay version, overlay analysis is not guaranteed to work. If the pack overlay version is higher than the CLI overlay version, overlay analysis is also not guaranteed to work.

We don't plan to ever reduce overlay versions with a new CodeQL CLI release, so the most common case would be the CLI having a higher overlay version that the pack. But I think the error message works both ways:

The query pack at ${packDir} was compiled with overlay version 6, but the CodeQL CLI supports only overlay version 4. The query pack needs to be recompiled to support overlay analysis.

and

The query pack at ${packDir} was compiled with overlay version 4, but the CodeQL CLI supports only overlay version 6. The query pack needs to be recompiled to support overlay analysis.

Ah, I see. I think the real problem here is that "only" can be ambiguous and could either be interpreted as "the versions supported by this CLI go up to only version X right now and version Y is greater" or "the version X must be an exact match for Y". I had interpreted it as the former, while you meant the latter.

If you don't plan to support version mixing, then perhaps the wording could be improved to be less ambiguous. How about simply dropping the "only":

The query pack at ${packDir} was compiled with overlay version 6, but the CodeQL CLI supports overlay version 4. The query pack needs to be recompiled to support overlay analysis.

mbg · 2025-08-05T18:26:52Z

src/init.ts

@@ -78,21 +78,40 @@ export async function runInit(
  apiDetails: GitHubApiCombinedDetails,
  logger: Logger,
 ): Promise<TracerConfig | undefined> {
-  fs.mkdirSync(config.dbLocation, { recursive: true });


Just noting this for future reference: it doesn't look like dbLocation is used by generateRegistries, so moving this seems fine.

Furthermore, codeql database init generally expects the database path to be non-existent and reports an error when the database path already exists. (There are some exceptions, such as when initializing an overlay database.) So I would be very surprised if generateRegistries() writes anything into dbLocation.

mbg · 2025-08-05T18:36:03Z

src/init-action.ts

+    // To check custom query packs for compatibility with overlay analysis, we
+    // need to first initialize the database cluster, which downloads the
+    // user-specified custom query packs. But we also want to check custom query
+    // pack compatibility first, because database cluster initialization depends
+    // on the overlay database mode. The solution is to initialize the database
+    // cluster first, check custom query pack compatibility, and if we need to
+    // revert to `OverlayDatabaseMode.None`, re-initialize the database cluster
+    // with the new overlay database mode.


It seems like a good bit of effort in the last couple of commits here is spent on enabling this. I am sure you've already thought about this, but is there any way that we could change the overlay database mode for the database cluster after it has been initialised? Or alternatively, shuffle things around so that we can download the user-specified custom query packs?

I am trying to understand the design space here a bit better. I am also thinking about concurrent efforts we'll have to look into related to code quality, which might mean that we have to break up what happens when a database is initialised anyway. It might be that we can find a solution for both in one go depending on what timelines permit.

I responded in more detail in the internal tracking issue, but the short answer is that neither of the suggested approach would be practical. Nonetheless thank you for the suggestions!

Thanks for the responses. I am happy to keep this as-is since it shouldn't be a common occurrence, and responded to the internal issue with more detailed thoughts.

Copilot

Pull Request Overview

This PR adds query pack compatibility checking for overlay analysis in the CodeQL Action. The main purpose is to ensure that when overlay analysis is enabled, all query packs involved are compatible with the installed CodeQL CLI's overlay analysis support before proceeding.

Introduces a new checkPacksForOverlayCompatibility function that validates query pack overlay compatibility
Refactors the runInit function into runDatabaseInitCluster and updates the overlay compatibility checking workflow
Updates the minimum CodeQL overlay version requirement from "2.20.5" to "2.22.3"

Reviewed Changes

Copilot reviewed 20 out of 29 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/util.ts	Adds `getGeneratedSuitePath` helper function for query suite paths
src/testing-utils.ts	Updates mock functions to support `overlayVersion` parameter
src/overlay-database-utils.ts	Updates minimum CodeQL overlay version constant
src/init.ts	Adds overlay compatibility checking logic and refactors database initialization
src/init.test.ts	Adds comprehensive tests for overlay compatibility checking
src/init-action.ts	Updates main initialization flow to handle overlay compatibility checks
src/config-utils.test.ts	Updates test configuration to use new minimum overlay version
src/codeql.ts	Adds `overlayVersion` to `VersionInfo` and `resolveQueriesStartingPacks` method
src/analyze.ts	Uses new `getGeneratedSuitePath` helper function
src/analyze.test.ts	Removes unused `packDownload` mock

Comments suppressed due to low confidence (2)

src/init.ts:79

[nitpick] The parameter order in runDatabaseInitCluster is inconsistent with the original runInit function. Consider maintaining the same parameter order (codeql, config, sourceRoot, processName, then additional parameters) for better consistency and easier migration.

export async function runDatabaseInitCluster(
  databaseInitEnvironment: Record<string, string | undefined>,
  codeql: CodeQL,
  config: configUtils.Config,
  sourceRoot: string,
  processName: string | undefined,
  qlconfigFile: string | undefined,
  logger: Logger,
): Promise<void> {

src/init.ts:138

[nitpick] The interface QlPack is too generic and incomplete for representing a qlpack.yml file structure. Consider renaming it to QlPackMetadata or QlPackYaml to be more specific about its purpose.

interface QlPack {
  buildMetadata?: string;
}

Copilot · 2025-08-05T21:45:26Z

src/init.ts

+    const packOverlayVersion = packInfoFileContents.overlayVersion;
+    if (typeof packOverlayVersion !== "number") {
+      logger.warning(
+        `The query pack at ${packDir} is not compatible with overlay analysis.`,


The error message 'is not compatible with overlay analysis' is vague. Consider making it more specific, such as 'is missing the overlayVersion field in .packinfo' or 'has an invalid overlayVersion field in .packinfo' to help users understand exactly what's wrong.

Suggested change

`The query pack at ${packDir} is not compatible with overlay analysis.`,

`The query pack at ${packDir} has an invalid or missing overlayVersion field in .packinfo and is not compatible with overlay analysis.`,

Copilot · 2025-08-05T21:45:26Z

src/init.ts

+    const qlpackContents = yaml.load(
+      fs.readFileSync(qlpackPath, "utf8"),
+    ) as QlPack;
+    if (!qlpackContents.buildMetadata) {


The code assumes qlpack.yml exists and can be parsed as YAML, but doesn't handle the case where the file doesn't exist or contains invalid YAML. This could cause the function to throw an unhandled exception instead of logging a warning and returning false.

cklin · 2025-08-05T21:59:20Z

I responded to some of the comments, and will follow up on the remaining ones tomorrow.

cklin · 2025-08-06T20:19:47Z

I responded to all comments (responses to copilot-initiated comments might be visible only under the original copilot reviews).

Also rebased the PR against current main to resolve merge conflicts.

cklin · 2025-08-07T16:54:48Z

Another rebase to resolve merge conflicts.

cklin · 2025-08-08T13:21:27Z

@mbg Friendly ping

mbg

Thanks for addressing my feedback from the last review, and sorry it took longer than I'd like to re-review.

This continues to look good, thank you! I've followed up to a few of your responses and added a couple of suggestions for potential improvements on test coverage.

I am generally happy with this and there isn't anything that's really blocking. Have a look over the comments and see if you want to change anything in light of them. I will be happy to approve this after you've had a look.

mbg · 2025-08-07T11:35:08Z

src/init.ts

+      logger.warning(
+        `The query pack at ${packDir} was compiled with ` +
+          `overlay version ${packOverlayVersion}, but the CodeQL CLI ` +
+          `supports only overlay version ${codeQlOverlayVersion}. The ` +


Ah, I see. I think the real problem here is that "only" can be ambiguous and could either be interpreted as "the versions supported by this CLI go up to only version X right now and version Y is greater" or "the version X must be an exact match for Y". I had interpreted it as the former, while you meant the latter.

If you don't plan to support version mixing, then perhaps the wording could be improved to be less ambiguous. How about simply dropping the "only":

The query pack at ${packDir} was compiled with overlay version 6, but the CodeQL CLI supports overlay version 4. The query pack needs to be recompiled to support overlay analysis.

mbg · 2025-08-07T22:31:13Z

src/init.ts

+      logger.warning(
+        `The query pack at ${packDir} is not compatible with overlay analysis.`,
+      );


I see Copilot agreed with my comment on this 😅

It might not be useful for users to know this implementation detail, but it would be helpful for us when troubleshooting issues related to this. It might be clear to you (and perhaps even me) what this error means since we know where in the code it is raised, but someone else might need to review the code to figure that out.

Just to be clear, I don't care whether you explicitly mention overlayVersion in the error or not. But it would be good to have a message that points more clearly at there being something wrong with the file (for whatever reason) than a compatibility issue for an entirely unspecified reason.

mbg · 2025-08-07T22:34:21Z

src/init.ts

+
+    const packInfoFileContents = JSON.parse(
+      fs.readFileSync(packInfoPath, "utf8"),
+    );


On second thought, I think it's probably fine. You're right that whatever errors come out of here are probably distinguishable based on the result of util.getErrorMessage(e) in the existing catch block.

mbg · 2025-08-08T13:37:56Z

src/init.ts

+      logger.warning(
+        `The query pack at ${packDir} ` +
+          "does not have a .packinfo file. This pack is too old to support " +
+          "overlay analysis.",
+      );


I think your proposed change is good. Proposing a possible resolution, rather than suggesting a cause might allow most people to resolve this issue themselves if they run into it as well.

The main scenario I was thinking of is users who create their own query packs and (perhaps inadvertently) use an older CLI for that. That said, I am not sure whether users who create their own packs often bundle them, so as you say this is probably not a likely issue.

mbg · 2025-08-08T13:41:56Z

src/config-utils.test.ts

The PR removes resolveQueries and adds resolveQueriesStartingPacks. For the corresponding tests, the ones for resolveQueries are removed, but none are added for resolveQueriesStartingPacks. Do you think it would make sense to add some?

I did not remove any resolveQueries tests. What I removed was unnecessary resolveQueries and packDownload stubs in config loading tests. Those tests are still working the same way they used to: the removed resolveQueries and packDownload stubs has no effect on the tests because the tested code does not call either of those functions.

resolveQueriesStartingPacks is just a wrapper that calls the corresponding CodeQL CLI subcommand, and I don't think tests for that function would add much value.

mbg · 2025-08-08T13:50:35Z

src/init-action.ts

+    // To check custom query packs for compatibility with overlay analysis, we
+    // need to first initialize the database cluster, which downloads the
+    // user-specified custom query packs. But we also want to check custom query
+    // pack compatibility first, because database cluster initialization depends
+    // on the overlay database mode. The solution is to initialize the database
+    // cluster first, check custom query pack compatibility, and if we need to
+    // revert to `OverlayDatabaseMode.None`, re-initialize the database cluster
+    // with the new overlay database mode.


Thanks for the responses. I am happy to keep this as-is since it shouldn't be a common occurrence, and responded to the internal issue with more detailed thoughts.

mbg · 2025-08-08T13:57:17Z

src/init-action.ts

+      cleanupDatabaseClusterDirectory(config, logger, {
+        disableExistingDirectoryWarning: true,
+      });
+      await runDatabaseInitCluster(
+        databaseInitEnvironment,
+        codeql,
+        config,
+        sourceRoot,
+        "Runner.Worker.exe",
+        qlconfigFile,
+        logger,
+      );


Do you think it's worth adding a test that exercises this logic? I don't see anything wrong with things as they are, but it might be worthwhile to have to check that we don't break this sequence, especially since an error in cleanupDatabaseClusterDirectory would be fatal. I don't feel strongly about it.

I added a new PR check to exercise this re-initialization logic.

This commit inlines runInit(), so that it is easier to repeat the runDatabaseInitCluster() call when needed.

cklin · 2025-08-08T17:13:17Z

@mbg Thanks for your comments. I think I responded to them all.

There has been a few force pushes to the PR branch, both to resolve merge conflicts and to iterate on the new database re-init PR check. The following link contains all the relevant changes since your last review:

https://github.com/github/codeql-action/compare/37f26d0f026df3e20f15eb7bf68a3bb8d096418f..e47147711bab2cae6089f637baa13b51de265126

Edit: I updated the comparison link because I added a commit to support the codeql-pack.yml pack file name.

cklin force-pushed the cklin/overlay-pack-check branch 4 times, most recently from 230fc32 to d0d4112 Compare July 29, 2025 18:58

cklin marked this pull request as ready for review July 29, 2025 19:20

Copilot AI review requested due to automatic review settings July 29, 2025 19:20

cklin requested a review from a team as a code owner July 29, 2025 19:20

cklin requested review from mbg and alexet July 29, 2025 19:20

This comment was marked as outdated.

Sign in to view

alexet reviewed Jul 31, 2025

View reviewed changes

cklin marked this pull request as draft July 31, 2025 16:06

cklin force-pushed the cklin/overlay-pack-check branch from d0d4112 to f1a0360 Compare July 31, 2025 18:31

cklin marked this pull request as ready for review July 31, 2025 20:38

cklin requested review from alexet and Copilot July 31, 2025 20:38

cklin force-pushed the cklin/overlay-pack-check branch from f1a0360 to 119d629 Compare August 5, 2025 18:03

This comment was marked as outdated.

Sign in to view

mbg reviewed Aug 5, 2025

View reviewed changes

cklin force-pushed the cklin/overlay-pack-check branch from 119d629 to 7ba8b26 Compare August 5, 2025 21:08

Copilot AI review requested due to automatic review settings August 5, 2025 21:44

cklin force-pushed the cklin/overlay-pack-check branch from 7ba8b26 to 210aa6a Compare August 5, 2025 21:44

Copilot AI reviewed Aug 5, 2025

View reviewed changes

alexet previously approved these changes Aug 6, 2025

View reviewed changes

cklin dismissed alexet’s stale review via 7a53966 August 6, 2025 20:17

cklin force-pushed the cklin/overlay-pack-check branch from 210aa6a to 7a53966 Compare August 6, 2025 20:17

cklin force-pushed the cklin/overlay-pack-check branch from 7a53966 to f5b3a08 Compare August 7, 2025 16:54

cklin requested a review from mbg August 7, 2025 16:55

mbg reviewed Aug 8, 2025

View reviewed changes

cklin added 5 commits August 8, 2025 08:47

Update CODEQL_OVERLAY_MINIMUM_VERSION

6504a72

VersionInfo: add overlayVersion field

da8dabf

Remove resolveQueries()

a71ebf3

Remove packDownload()

ad19982

Add resolveQueriesStartingPacks()

9d202e0

cklin force-pushed the cklin/overlay-pack-check branch from f5b3a08 to 37f26d0 Compare August 8, 2025 15:50

cklin added 6 commits August 8, 2025 08:53

Add checkPacksForOverlayCompatibility()

c47e154

Extract runDatabaseInitCluster()

ed9d057

Inline runInit()

ee7cacd

This commit inlines runInit(), so that it is easier to repeat the runDatabaseInitCluster() call when needed.

init-action: check packs for overlay compatibility

bc9c32e

init-action: inhibit non-empty dbLocation warning when restarting

ee698cb

Extract getGeneratedSuitePath()

8dbcef5

cklin force-pushed the cklin/overlay-pack-check branch 2 times, most recently from f17b8c2 to a81656b Compare August 8, 2025 16:50

PR checks: add overlay-init-fallback.yml

57f4ac5

cklin force-pushed the cklin/overlay-pack-check branch from a81656b to 2b2e27a Compare August 8, 2025 16:58

cklin requested a review from mbg August 8, 2025 17:13

cklin added 2 commits August 8, 2025 10:34

Check both qlpack.yml and codeql-pack.yml

baac929

build: refresh js files

e471477

cklin force-pushed the cklin/overlay-pack-check branch from 2b2e27a to e471477 Compare August 8, 2025 17:36

	`The query pack at ${packDir} is not compatible with overlay analysis.`,
	`The query pack at ${packDir} has an invalid or missing overlayVersion field in .packinfo and is not compatible with overlay analysis.`,

Overlay: check query packs for compatibility #2993

Are you sure you want to change the base?

Overlay: check query packs for compatibility #2993

Uh oh!

Conversation

cklin commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge / deployment checklist

Uh oh!

This comment was marked as outdated.

Uh oh!

alexet left a comment

Choose a reason for hiding this comment

Uh oh!

cklin commented Jul 31, 2025

Uh oh!

cklin commented Jul 31, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

cklin commented Aug 5, 2025

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

cklin commented Aug 5, 2025

cklin commented Jul 25, 2025 •

edited

Loading

cklin commented Aug 8, 2025 •

edited

Loading