Basic support for overlay PR analysis by cklin · Pull Request #2945 · github/codeql-action · GitHub

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.

Pull Request Overview

Adds foundational support for overlay database analysis, including caching of the overlay-base database and integration into init/analyze workflows.

Introduces functions to upload/download overlay-base DB to Actions cache.
Updates configuration logic to determine overlay modes and caching behavior.
Integrates overlay mode handling into init and analyze actions and feature flags.

Reviewed Changes

Copilot reviewed 28 out of 42 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/overlay-database-utils.ts	Implements check, upload, and download for overlay-base DB cache
src/config-utils.ts	Adds getOverlayDatabaseMode and new augmentation properties
src/init-action.ts	Passes sourceRoot to init and applies cache download logic
src/analyze-action.ts	Overrides cleanup level and uploads overlay-base DB to cache
src/feature-flags.ts	Introduces OverlayAnalysis feature and min CLI version check

Comments suppressed due to low confidence (2)

src/overlay-database-utils.ts:170

The new overlay database caching functions (uploadOverlayBaseDatabaseToCache and downloadOverlayBaseDatabaseFromCache) are not covered by existing tests. Consider adding unit tests to validate their guard conditions and successful cache interactions.

export async function uploadOverlayBaseDatabaseToCache(

src/config-utils.ts:791

[nitpick] getOverlayDatabaseMode contains complex branching logic. Consider adding or expanding its JSDoc to outline the decision flow and environment variable precedence for better readability and future reference.

async function getOverlayDatabaseMode(

[nitpick] The guard logic for overlay database caching (checking mode, caching flag, and test mode) is duplicated in both upload and download functions. Consider extracting this into a shared helper to reduce duplication and simplify future maintenance.

I have a few questions/comments, but overall it looks very sensible.

I think we need to check whether PR analyses for dynamic languages like Ruby actually have the build mode defined as None, or if it's left undefined.

I updated the code so that it checks for build mode only for traced languages.

Can you explain why we can't just use the feature flag to restrict overlay analysis to these orgs?

This PR does not fully implement all the functionality that we planned, so it is considered an early prototype and suitable for internal testing only. So we want to be sure that we never attempt to perform overlay analysis on user repositories with this implementation, even if a user were to pin their workflow to this PR after merge, and then we enable all feature flags.

That is why I am using an explicit allowlist here instead of relying on feature flags.

this seems to be worth a TODO comment 🙂

There is also a potential concern here for outside of dotcom if this ends up in a release, since it is possible for customers on other instances to have organisations with specific names. Two options:

If you want to get this merged, then I think it would be good to put this behind a FF now, which then also helps us going forward.
If it's really for internal testing only, then you may not need to get this PR merged at all, if you are able to reference this branch directly in testing workflows

Sorry, I only reviewing this fully now, so I think I missed that you have already added the FF and reasoned why you should have both checks. I am not sure we should really be concerned with users pinning feature branches. That's not a concern for default setup, and if they did it with an advanced setup, then they are probably aware that it is at their own risk (they could also just enable the environment variable).

this seems to be worth a TODO comment 🙂

Yes, that is a good suggestion. I will do that.

There is also a potential concern here for outside of dotcom if this ends up in a release, since it is possible for customers on other instances to have organisations with specific names.

Thanks for bringing that up—I haven't considered that possibility. That said, we do have a feature flag on top of the allow list (your option 1 above), so GHES users should never reach this check.

If it's really for internal testing only, then you may not need to get this PR merged at all, if you are able to reference this branch directly in testing workflows

The internal testing can include staff-ship to internal repositories, and it would be ideal if we can do so through feature flags without requiring workflow file changes.

I am not sure we should really be concerned with users pinning feature branches.

Once this PR merges, a user could get this code by pinning a commit on main, which I think is a use case we should support.

The internal testing can include staff-ship to internal repositories, and it would be ideal if we can do so through feature flags without requiring workflow file changes.

Agreed.

Once this PR merges, a user could get this code by pinning a commit on main, which I think is a use case we should support.

I am a bit conflicted about this. On one hand, I agree that a user could do this and the "future FF enablement on an old pinned version" scenario is an interesting one to consider. On the other hand, I am not sure how much we have considered this scenario in the past -- possibly a question for @aeisenberg or @henrymercer. I'm slightly leaning towards thinking that the FF alone is probably enough, particularly since a user who pins this version could also just set the environment variable to get this prototype implementation, but no strong feelings.

FYI we also have a FF in default setup for internal things. That isn't propagated to the action right now in any way, but it could be. Perhaps if we feel that this kind of feature gating is necessary for experimental features, then we could instead do some work to start using that.

If the GITHUB_SHA env var is somehow not set, should we instead skip uploading to the actions cache?

Good point. In addition to what you said, there is another reason why we should not be relying on GITHUB_SHA: the environment variable, even if it exists, may not necessarily correspond to the commit that is analyzed. It is better to call getCommitOid() instead.

I updated the code to use getCommitOid().

I don't follow how this can work, since the saveKey computation appends either a SHA or unknown to the result of getCacheRestoreKey(), and we're not appending that suffix here.

This code relies on the actions cache feature of performing restores using prefix match on the cache key:
https://docs.github.com/en/actions/how-tos/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows#matching-a-cache-key

Thank you for your feedback @nickrolfe!

I will take the PR back to draft to address comments and resolve merge conflicts. Once that is done, I will mark the PR as ready for review again.

@nickrolfe The PR is ready for another look.

Wouldn't it be better to raise a configuration error if we explicitly set anything else than overlay or nothing if we are using overlay database? I think we shouldn't silently ignore an explicit parameter by the user

I agree with @redsun82. It might be surprising if the cleanup-level option is silently overriden. An error if cleanup-level is set and overlay databases are enabled would make sense.

I am a bit conflicted about what to do here.

On the one hand, silently ignoring explicit user input (which I am doing here) is not considered proper behavior.

On the other hand, it is important for the action to maintain backward compatibility. If we were to throw an error for users setting cleanup-level to a value other than overlay, we would break those (currently working) workflows when we roll out overlay analysis. I can't think of any reason for users to set cleanup-level to overlay, especially since it is currently not even a documented allowed value.

Another option is to not build an overlay-base database if the user specifies a cleanup-level that is not overlay. There is no good way to do that because the decision to build the overlay-base database was made in the init step, and the cleanup-level input is in the analyze step. We could obey the cleanup-level input and not upload the overlay-base database if the input is not overlay, which means that the workflow will not benefit from overlay analysis. It becomes a matter of which we think the user would care more about: getting the benefit of overlay analysis when we roll it out, or having the cleanup-level input strictly obeyed?

Of these three options, I think the currently implemented behavior is the best choice. We can make the behavior change more explicit, by updating the cleanup-level description to state that it is advisory, and (if you consider it appropriate) to add a changelog entry stating the same. At the end of the day, the original assumption when we added cleanup-level (that database cleanup is a choice that has no impact on how the action itself functions) will no longer hold, and I think it is not unreasonable for the meaning of the input to change accordingly.

Thanks for the thoughtful discussion of this!

Firstly, I am thinking that, regardless of which behaviour we go with, we should implement a warning that warns of incompatible cleanup-level and overlay database settings.

Beyond that, my feeling is that we should preserve existing behaviour, at least until we have given enough notice that a change is coming. That is to say, I'd probably have a preference for "We could obey the cleanup-level input and not upload the overlay-base database if the input is not overlay, which means that the workflow will not benefit from overlay analysis.".

I would say that is acceptable because:

We will have a warning which notifies the user (but not an error that breaks existing workflows).
We can assume that cleanup-level isn't super commonly used, and most users will still benefit from overlay databases anyway, particularly in default setup where no cleanup-level is specified.
We don't override user-configured behaviour.
Since cleanup-level is used in advanced setup, users can change this setting if they have it configured and they want to take advantage of overlay databases.

We can then, after some time, revisit this to decide whether we want to override the cleanup-level setting if overlay databases are enabled.

this seems to be worth a TODO comment 🙂

Theoretically as stated on line 86, we should

Prefer using ToolsFeatures for future flags

Maybe we're not very thorough with that, and as far as I can see we did not add overlayDatabases to the list of CLI features that are listed in codeql version --format=json. We should have, right?

Agreed that it would be better to use the CLI's features list than set a minimumVersion here.

The CLI interface for overlay analysis has not yet been completely finalized, which is one of the reasons why we have both the feature flag check and the explicit allowlist for internal orgs. We can use ToolsFeatures, but that would require us to come up with a new ToolsFeature if there should be an upcoming change that is not backwards compatible. Setting a minimum version number makes it easier to iterate.

There is also the question of how much we want to lean on the version or feature check. Support for overlay analysis is not just a feature of the CLI—it is a feature of the CLI and the compiled queries. ToolsFeature tells us whether we can pass the overlay analysis flags to the CLI and not get an error. But it does not tell us whether the query packs are ready for overlay analysis. By setting a minimum version number, we can version not only the CLI but also the precompiled query packs included in the bundle, which helps us avoid unintended enablement.

(In the long run, we do plan to make the CLI robust enough so that it can correctly handle existing pre-compiled query packs for overlay analysis. For now internal testing focuses on query packs included in the bundle, and the minimum version serves as a useful check.)

I think I am happy with that justification for leaving this as is for now.

I re-reviewed the parts I previously commented on, and I'm happy with the changes.

Thank you for preparing this PR -- I am very excited for this functionality! 💖

As I suggested in my earlier comment, if the main goal is to test this internally for now, then it may be best to leave this PR unmerged and reference the PR branch explicitly in test workflows. Have you done any testing like that yet? If not, it would be good to do some of that in any case.

If we want to merge this, then the changes will end up in the next release. Before we do that, I think we should address a few points:

Before we merge this, we should add some test coverage. At this stage, it would be good to gain confidence that none of the new logic interferes with existing functionality when the feature is disabled. The existing PR checks do that to an extent, but we should verify that we cover all cases. In the long run, we will want test coverage for when the feature is enabled as well.
There are general concerns around us using the Actions cache, similar to what we have for TRAP caching and dependency caching. I.e. we might be eating away at the cache quota that may already be in use by other workflows in a given repository. Can we mitigate our impact here? For example, by ensuring that we only have one cache at a time or limiting the size of the caches we store. This may not need to be addressed at this point if we only plan to use this internally for now, but it should be addressed before we ship it more widely, especially if the goal is to have it on by default at some point.

I agree with @redsun82. It might be surprising if the cleanup-level option is silently overriden. An error if cleanup-level is set and overlay databases are enabled would make sense.

Agreed that it would be better to use the CLI's features list than set a minimumVersion here.

Sorry, I only reviewing this fully now, so I think I missed that you have already added the FF and reasoned why you should have both checks. I am not sure we should really be concerned with users pinning feature branches. That's not a concern for default setup, and if they did it with an advanced setup, then they are probably aware that it is at their own risk (they could also just enable the environment variable).

Henry has an open PR to open up languages more, which might affect this in the future. Not necessarily something we need to consider here, but may need to consider there.

@mbg Thank you for your review!

Before we merge this, we should add some test coverage. At this stage, it would be good to gain confidence that none of the new logic interferes with existing functionality when the feature is disabled. The existing PR checks do that to an extent, but we should verify that we cover all cases. In the long run, we will want test coverage for when the feature is enabled as well.

I agree that more tests are a good idea. Can you say more about what you mean by "gain confidence that none of the new logic interferes with existing functionality" and "cover all cases", so that I have a better estimate of the effort required?

There are general concerns around us using the Actions cache, similar to what we have for TRAP caching and dependency caching. I.e. we might be eating away at the cache quota that may already be in use by other workflows in a given repository. Can we mitigate our impact here? For example, by ensuring that we only have one cache at a time or limiting the size of the caches we store. This may not need to be addressed at this point if we only plan to use this internally for now, but it should be addressed before we ship it more widely, especially if the goal is to have it on by default at some point.

Thank you for bringing that up. I added a note to the internal tracking issue for consideration later.

Can you say more about what you mean by "gain confidence that none of the new logic interferes with existing functionality" and "cover all cases".

Broadly, we should check that getOverlayDatabaseMode() doesn't turn on this feature unexpectedly. I.e. we should have tests that cover all the cases it uses for deciding whether to turn the feature on or not. That might be doable with some unit tests, or might need an extra pr-check.

This commit adds overlayDatabaseMode to AugmentationProperties and creates a placeholder getOverlayDatabaseMode() function, with the necessary inputs, to populate it.

This commit populates getOverlayDatabaseMode() in config-utils with the same code from getOverlayDatabaseMode() in init.

This commit changes databaseInitCluster() to use overlayDatabaseMode from AugmentationProperties instead of the overlayDatabaseMode parameter. There is no behavior change because both overlayDatabaseMode values are computed the same way. The commit then cleans up the overlayDatabaseMode parameter and the code paths that feed into it.

This commit changes getOverlayDatabaseMode so that, when Feature.OverlayAnalysis is enabled, it calculates the overlay database mode automatically based on analysis metadata. If we are analyzing the default branch, use OverlayBase, and if we are analyzing a PR, use Overlay. If CODEQL_OVERLAY_DATABASE_MODE is set to a valid overlay database mode, that environment variable still takes precedence.

This commit adds useOverlayDatabaseCaching to AugmentationProperties to indicate whether the action should upload overlay-base databases to the actions cache and to download a cached overlay-base database when creating an overlay database.

@mbg

I made some of the requested changes (diff from force pushes sans rebase).

Added comment on sourceRoot definition
Added comment on repo owner allowlist
Added unit tests for getPullRequestBranches()
Added unit tests for getOverlayDatabaseMode()

These are the changes I consider (a) in-scope for this PR and (b) there is clear consensus on how to proceed.

The main unresolved issue is how we want to handle the cleanup-level input. My reading of the internal discussion is that that input is considered advisory, and it is reasonable to override the user input when appropriate. That said, I am happy to make additional changes to try to mitigate the situation:

Update the description of cleanup-level to specify that it is advisory
Remove the default brutal value of cleanup-level, and document the default as automatic selection
Emit a warning when we override the user-specified cleanup-level value

What do you think?

Thanks for making these changes, the new tests are nicely implemented! ✨

I think I am happy to approve this in its current state. I only had a couple of minor thoughts about the tests, which are not urgent. I have also responded to the point about cleanup-level below, where one of the points you listed could be done as part of this PR.

The main unresolved issue is how we want to handle the cleanup-level input. My reading of the internal discussion is that that input is considered advisory, and it is reasonable to override the user input when appropriate.

I think we should consider this a bit more before reaching a decision. We can proceed with this PR though, since it is an experimental implementation for internal testing that's gated behind an allow list and FF. However, I would like to refrain from making changes that affect all users until we have reached a decision.

The current implementation only overrides the cleanup-level if overlay databases are enabled, so it might be OK to mostly leave that as-is for now. Implementing the warning if the cleanup-level gets overridden would probably still be useful though.

If we decide to go ahead with public-facing changes to the cleanup-level input, then we can make those in a separate PR.

Minor: Any reason for saving the entire context even though only the payload needs to be restored later?

Thanks—I implemented the suggested change.

Minor: perhaps it would be good to have a test case for a non-pull_request event (e.g. push) to check that isAnalyzingPullRequest returns false there.

Thanks—I implemented the suggested change.

cklin marked this pull request as ready for review June 23, 2025 15:51

Copilot AI review requested due to automatic review settings June 23, 2025 15:51

cklin requested a review from a team as a code owner June 23, 2025 15:51

cklin assigned nickrolfe Jun 23, 2025

cklin requested a review from nickrolfe June 23, 2025 15:51

Copilot AI reviewed Jun 23, 2025

View reviewed changes

nickrolfe reviewed Jun 26, 2025

View reviewed changes

cklin marked this pull request as draft June 30, 2025 21:05

cklin force-pushed the cklin/overlay-analysis branch from fd2660c to bc3a47b Compare July 1, 2025 17:50

cklin marked this pull request as ready for review July 1, 2025 18:19

redsun82 reviewed Jul 2, 2025

View reviewed changes

nickrolfe previously approved these changes Jul 2, 2025

View reviewed changes

mbg reviewed Jul 2, 2025

View reviewed changes

cklin added 13 commits July 3, 2025 12:35

Add AugmentationProperties.overlayDatabaseMode

9022c73

This commit adds overlayDatabaseMode to AugmentationProperties and creates a placeholder getOverlayDatabaseMode() function, with the necessary inputs, to populate it.

config-utils: populate getOverlayDatabaseMode()

ee8a8c4

This commit populates getOverlayDatabaseMode() in config-utils with the same code from getOverlayDatabaseMode() in init.

Add isAnalyzingPullRequest()

60a2a7d

Add Feature.OverlayAnalysis

da758dc

Limit OverlayAnalysis to internal repos

b442537

Upload overlay-base database to actions cache

6ca06f4

Extract checkOverlayBaseDatabase()

b95402d

Download overlay-base database from actions cache

2fc04c8

Override cleanup-level for overlay-base database

42835b3

Add "overlay" to SARIF incrementalMode run property

6a51e63

cklin dismissed nickrolfe’s stale review via a8a0cc8 July 3, 2025 19:38

cklin force-pushed the cklin/overlay-analysis branch 3 times, most recently from f32db97 to 55ea4c3 Compare July 3, 2025 20:32

mbg previously approved these changes Jul 7, 2025

View reviewed changes

cklin added 3 commits July 7, 2025 08:13

Add getPullRequestBranches() tests

8c5122e

Add getOverlayDatabaseMode() tests

95a1b7e

build: refresh js files

ec836d6

cklin dismissed mbg’s stale review via ec836d6 July 7, 2025 15:15

cklin force-pushed the cklin/overlay-analysis branch from 55ea4c3 to ec836d6 Compare July 7, 2025 15:15

mbg approved these changes Jul 7, 2025

View reviewed changes

cklin merged commit 624d0bc into main Jul 7, 2025

cklin deleted the cklin/overlay-analysis branch July 7, 2025 15:41

cklin mentioned this pull request Jul 7, 2025

Revert "Basic support for overlay PR analysis" #2962

Closed

github-actions Bot mentioned this pull request Jul 21, 2025

Merge main into releases/v3 #2977

Merged

8 tasks

+                logger: Logger,
+              ): Promise<boolean> {
+                const overlayDatabaseMode = config.augmentationProperties.overlayDatabaseMode;
+                if (overlayDatabaseMode !== OverlayDatabaseMode.OverlayBase) {

+                if (buildMode !== BuildMode.None) {
+                  logger.warning(
+                    `Cannot build an ${overlayDatabaseMode} database because ` +
+                      `build-mode is set to "${buildMode}" instead of "none". ` +
+                      "Falling back to creating a normal full database instead.",
+                  );
+                  return nonOverlayAnalysis;

+                      "from the CODEQL_OVERLAY_DATABASE_MODE environment variable.",
+                  );
+                } else if (
+                  ["github", "dsp-testing"].includes(repository.owner) &&

+                  // An overlay-base database should always use the 'overlay' cleanup level
+                  // to preserve the cached intermediate results.
+                  //
+                  // Note that we may be overriding the 'cleanup-level' input parameter.
                   const cleanupLevel =
-                    actionsUtil.getOptionalInput("cleanup-level") || "brutal";
+                    config.augmentationProperties.overlayDatabaseMode ===
+                    OverlayDatabaseMode.OverlayBase
+                      ? "overlay"
+                      : actionsUtil.getOptionalInput("cleanup-level") || "brutal";

+                      "from the CODEQL_OVERLAY_DATABASE_MODE environment variable.",
+                  );
+                } else if (
+                  ["github", "dsp-testing"].includes(repository.owner) &&

+              }
+              function generateCacheKey(config: Config, codeQlVersion: string): string {
+                const sha = process.env.GITHUB_SHA || "unknown";

+                const dbLocation = config.dbLocation;
+                const codeQlVersion = (await codeql.getVersion()).version;
+                const restoreKey = getCacheRestoreKey(config, codeQlVersion);

+                [Feature.OverlayAnalysis]: {
+                  defaultValue: false,
+                  envVar: "CODEQL_ACTION_OVERLAY_ANALYSIS",
+                  minimumVersion: CODEQL_OVERLAY_MINIMUM_VERSION,

+                // we are analyzing. However, since overlay-base databases are built from the
+                // default branch and used in PR analysis, it is exceedingly unlikely that
+                // the commit SHA will ever be the same, so we can just leave it out.
+                const languages = [...config.languages].sort().join("_");

               setupTests(test);
+              function withMockedContext<T>(mockPayload: any, testFn: () => T): T {
+                const originalContext = github.context;

                 );
               });
+              test("getPullRequestBranches() with pull request context", (t) => {

Basic support for overlay PR analysis by cklin · Pull Request #2945 · github/codeql-action · GitHub

Uh oh!

Basic support for overlay PR analysis - #2945

Basic support for overlay PR analysis#2945cklin merged 16 commits intomaingithub/codeql-action:mainfrom cklin/overlay-analysisgithub/codeql-action:cklin/overlay-analysisCopy head branch name to clipboard

Conversation

cklin commented Jun 23, 2025

Merge / deployment checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

nickrolfe left a comment

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!

cklin commented Jun 30, 2025

Uh oh!

cklin commented Jul 1, 2025

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!

redsun82 Jul 2, 2025 • 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!

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!

nickrolfe left a comment

Choose a reason for hiding this comment

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!

Basic support for overlay PR analysis#2945
cklin merged 16 commits into
mainfrom
cklin/overlay-analysis

redsun82 Jul 2, 2025 •
edited
Loading