Overlay databases: Use `Config` instead of `AugmentationProperties` by mbg · Pull Request #3072 · github/codeql-action · GitHub

Extracted from #3064. Moves extraQueryExclusions and overlay database configuration out of AugmentationProperties and into Config.

The reasoning for this change is that AugmentationProperties is intended for user inputs to the init Action that cannot be represented in the same format that is used by UserConfig (which is understood by the CodeQL CLI). Options stored in the AugmentationProperties are then intended to be combined with an existing UserConfig into a new UserConfig.

We will remove AugmentationProperties from Config (used to represent the CodeQL Action configuration / state that is shared across different steps in a CodeQL workflow) in #3075. This makes sense, because the purpose of AugmentationProperties (as explained above) is just to represent user inputs to the Action until they can be converted into a UserConfig.

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.

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.

Pull Request Overview

This PR refactors the overlay database configuration by moving related properties from the AugmentationProperties interface to the main Config interface for better organization and clarity.

Key Changes:

Moves overlay database mode and caching properties from AugmentationProperties to Config
Adds computedConfig field to store the final computed configuration
Updates all references to use the new property locations

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/config-utils.ts	Moves overlay database properties from AugmentationProperties to Config, adds computedConfig field
src/testing-utils.ts	Updates test configuration to use new property locations
src/overlay-database-utils.ts	Updates overlay database utility functions to access properties from config instead of config.augmentationProperties
src/init-action.ts	Updates initialization code to use new property structure
src/analyze.ts	Updates analysis functions to use new property locations
src/codeql.ts	Updates CodeQL functions to use new property structure and removes dependency on generateCodeScanningConfig
src/test.ts	Updates test files to use new property structure
lib/*.js	Generated JavaScript files with corresponding changes

We need to be a careful here. Ordering matters in query-filters, and there was a recent bug where we accidentally overrode the user-specified query-filters setting by putting our own exclusion at the top of the list. See comments at

codeql-action/src/config-utils.ts

Lines 1478 to 1485 in 68d7fe3

    
           augmentedConfig["query-filters"] = [ 
        
             // Ordering matters. If the first filter is an inclusion, it implicitly 
        
             // excludes all queries that are not included. If it is an exclusion, 
        
             // it implicitly includes all queries that are not excluded. So user 
        
             // filters (if any) should always be first to preserve intent. 
        
             ...(augmentedConfig["query-filters"] || []), 
        
             ...augmentationProperties.extraQueryExclusions, 
        
           ];

Saving the programmatic exclusions in extraQueryExclusions allows us to easily ensure that those exclusions always come after user-specified filters. If we are mixing them all together in the same ``query-filters` field, we would need some other way to ensure proper ordering.

Yep, I am aware that the ordering here is critical and that you previously fixed an issue related to that.

For context, the main motivation for this PR is that the overlay database settings don't really belong in AugmentationProperties and we are working on removing the AugmentationProperties in Config.

In other words, the line you commented on does not produce a UserConfig that's later combined with other AugmentationProperties using generateCodeScanningConfig. The UserConfig that is mutated on the line you've commented on is the UserConfig that is given to the CLI. Since we are appending exclude-from-incremental here, it should have the same effect as what you previously accomplished by modifying AugmentationProperties.

First, thank you for the explanation!

There are two parts to my concern.

That this PR preserves the existing behavior of putting the exclude-from-incremental exclusion after user-specified query filters
That we help future developers (which could well be our future selves) avoid introducing this regression by accident

You made a strong case that you addressed the first part, and I agree with you on that. What about the second part?

Thanks for separating those points. I think what 2. has made me realise here is that you had introduced extraQueryExclusions to AugmentationProperties as part of your work on the Action in the last few months, but that it also doesn't really belong into AugmentationProperties.

What I could propose then to address 2. is to move extraQueryExclusions to Config as well, add exclude-from-incremental to it here, and then apply extraQueryExclusions to the query-filters of the UserConfig that we pass on to the CLI as late as possible (i.e. just before invoking the CLI). That would then behave similarly to what we had previously, except we don't have the baggage of the entire AugmentationProperties.

There might be more we could do to protect against shooting ourselves in the foot, but I think that's largely outside the scope of this change.

I have pushed a commit which moves extraQueryExclusions out of AugmentationProperties into Config. The part of the code from generateCodeScanningConfig that appends the extraQueryExclusions to a UserConfig is now in its own function as well. I have added a bit more documentation at the point where we append the extra query exclusions that links to your previous PR as well.

Thank you! These changes addressed all my concerns.

…further modification

ok

mbg force-pushed the mbg/refactor/augmentation-properties branch 2 times, most recently from d0ba608 to 029e76e Compare September 2, 2025 12:59

mbg marked this pull request as ready for review September 2, 2025 13:06

mbg requested a review from a team as a code owner September 2, 2025 13:06

mbg requested review from Copilot and henrymercer September 2, 2025 13:07

Copilot AI reviewed Sep 2, 2025

View reviewed changes

mbg requested a review from cklin September 2, 2025 13:24

cklin reviewed Sep 2, 2025

View reviewed changes

mbg added 2 commits September 2, 2025 18:01

Add computedConfig property to Config type

c4d2772

Move overlay db settings from AugmentationProperties to Config

ac9b91e

mbg force-pushed the mbg/refactor/augmentation-properties branch from 029e76e to ac9b91e Compare September 2, 2025 17:02

mbg mentioned this pull request Sep 2, 2025

Remove augmentationProperties from Config type #3075

Merged

mbg requested a review from cklin September 3, 2025 11:08

mbg force-pushed the mbg/refactor/augmentation-properties branch from e4b1ce5 to fe428a8 Compare September 3, 2025 11:10

Move extraQueryExclusions out of AugmentationProperties

e9fb72d

mbg force-pushed the mbg/refactor/augmentation-properties branch from fe428a8 to e9fb72d Compare September 3, 2025 11:14

Mark result of appendExtraQueryExclusions as Readonly to prevent …

7f81363

…further modification

mbg force-pushed the mbg/refactor/augmentation-properties branch from 61c593a to 7f81363 Compare September 3, 2025 11:21

henrymercer approved these changes Sep 3, 2025

View reviewed changes

cklin approved these changes Sep 3, 2025

View reviewed changes

mbg merged commit 1fd8a71 into main Sep 3, 2025
293 of 294 checks passed

mbg deleted the mbg/refactor/augmentation-properties branch September 3, 2025 15:40

github-actions Bot mentioned this pull request Sep 5, 2025

Merge main into releases/v3 #3081

Merged

8 tasks

krsjenswbp reviewed Sep 11, 2025

View reviewed changes

Overlay databases: Use `Config` instead of `AugmentationProperties` by mbg · Pull Request #3072 · github/codeql-action · GitHub

Uh oh!

Overlay databases: Use Config instead of AugmentationProperties - #3072

Overlay databases: Use Config instead of AugmentationProperties#3072
mbg merged 4 commits into
mainfrom
mbg/refactor/augmentation-properties

mbg commented Sep 2, 2025 •
edited
Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

cklin Sep 2, 2025

Uh oh!

mbg Sep 2, 2025

Uh oh!

cklin Sep 2, 2025

Uh oh!

mbg Sep 2, 2025

Uh oh!

mbg Sep 3, 2025

Uh oh!

cklin Sep 3, 2025

Uh oh!

Uh oh!

krsjenswbp left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

+                  if (config.computedConfig["query-filters"] === undefined) {
+                    config.computedConfig["query-filters"] = [];
+                  }
+                  config.computedConfig["query-filters"].push({

	augmentedConfig["query-filters"] = [
	// Ordering matters. If the first filter is an inclusion, it implicitly
	// excludes all queries that are not included. If it is an exclusion,
	// it implicitly includes all queries that are not excluded. So user
	// filters (if any) should always be first to preserve intent.
	...(augmentedConfig["query-filters"] \|\| []),
	...augmentationProperties.extraQueryExclusions,
	];

Overlay databases: Use `Config` instead of `AugmentationProperties` by mbg · Pull Request #3072 · github/codeql-action · GitHub

Uh oh!

Overlay databases: Use Config instead of AugmentationProperties - #3072

Overlay databases: Use Config instead of AugmentationProperties#3072mbg merged 4 commits intomaingithub/codeql-action:mainfrom mbg/refactor/augmentation-propertiesgithub/codeql-action:mbg/refactor/augmentation-propertiesCopy head branch name to clipboard

Conversation

mbg commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Risk assessment

Merge / deployment checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes:

Reviewed Changes

Uh oh!

cklin Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

mbg Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

cklin Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

mbg Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

mbg Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

cklin Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krsjenswbp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Overlay databases: Use Config instead of AugmentationProperties#3072
mbg merged 4 commits into
mainfrom
mbg/refactor/augmentation-properties

mbg commented Sep 2, 2025 •
edited
Loading