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.

src/init.ts

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.

src/init.ts

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

src/init.ts

  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.

src/init-action.ts

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;
}

src/init.ts

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-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.

src/init.ts

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.

src/init.ts

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.

src/init-action.ts

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.

mbg

Thanks for making those final changes!

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 alexet and mbg 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

src/init.ts Outdated Show resolved Hide resolved

src/init.ts Show resolved Hide resolved

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

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

mbg previously approved these changes Aug 11, 2025

View reviewed changes

Merge branch 'main' into cklin/overlay-pack-check

821d3bd

cklin dismissed mbg’s stale review via 821d3bd August 11, 2025 14:10

mbg approved these changes Aug 11, 2025

View reviewed changes

cklin merged commit c6dcdfa into main Aug 11, 2025
282 checks passed

cklin deleted the cklin/overlay-pack-check branch August 11, 2025 14:42

github-actions bot mentioned this pull request Aug 12, 2025

Merge main into releases/v3 #3026

Merged

8 tasks

Overlay: check query packs for compatibility #2993

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

cklin commented Aug 5, 2025

Uh oh!

cklin commented Aug 8, 2025

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

cklin commented Jul 25, 2025 •

edited

Loading

cklin commented Aug 8, 2025 •

edited

Loading