Overlay: additional feature flags #2967

cklin · 2025-07-10T21:46:58Z

This PR updates the action to check additional per-language features for enabling overlay analysis.

See back-linked issue for additional context.

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.

The loadConfig() function is mostly the same as getDefaultConfig(), except that it calls loadUserConfig() and stores the results in originalUserInput. This refactoring commit replaces the loadConfig() call with getDefaultConfig() and loadUserConfig(), which allows deleting a large amount of duplicated code.

In an upcoming change, getOverlayDatabaseMode() will depend on the contents of Config. As a result, getOverlayDatabaseMode() needs to be called after the rest of Config has already been populated. This commit performs the refactoring to move the getOverlayDatabaseMode() into initConfig(), after the rest of Config has already been populated.

Before we introduce additional features for controlling overlay analysis enablement, change the unit tests to specify features directly instead of through a isFeatureEnabled boolean field.

This change eliminates the need for setup-codeql.test to import from feature-flags.test, which makes the former run all tests defined in the latter.

Copilot

Pull Request Overview

This PR extends the overlay analysis feature to support more granular per-language enablement through additional feature flags. Instead of relying on a single overlay_analysis flag, the action now checks individual language-specific flags and code-scanning-specific flags to determine whether overlay analysis should be enabled.

Adds 22 new overlay analysis feature flags for language-specific and code-scanning-specific configurations
Implements chunked API requests to handle the increased number of feature flags (25 features per request)
Refactors configuration generation to better determine when overlay analysis is appropriate based on the selected languages and configuration

Reviewed Changes

Copilot reviewed 15 out of 22 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/feature-flags.ts	Adds new overlay analysis feature flags and implements chunked API requests
src/config-utils.ts	Adds language-specific overlay analysis logic and refactors config generation
src/testing-utils.ts	Moves utility functions and adds new test helpers for feature flag testing
src/setup-codeql.test.ts	Updates import to use testing-utils for initializeFeatures
src/feature-flags.test.ts	Adds test for chunked API requests and moves initializeFeatures
src/config-utils.test.ts	Adds comprehensive tests for new overlay analysis logic
src/codeql.ts	Refactors code scanning config generation to use shared utility
src/trap-caching.ts	Minor import style changes

Copilot · 2025-07-11T19:23:58Z

src/feature-flags.ts

+      while (featuresToRequest.length > 0) {
+        featureChunks.push(featuresToRequest.splice(0, FEATURES_PER_REQUEST));


The splice operation modifies the original array, which could lead to unexpected behavior if featuresToRequest is used elsewhere. Consider using a copy of the array or a different iteration approach.

Suggested change

while (featuresToRequest.length > 0) {

featureChunks.push(featuresToRequest.splice(0, FEATURES_PER_REQUEST));

for (let i = 0; i < featuresToRequest.length; i += FEATURES_PER_REQUEST) {

featureChunks.push(featuresToRequest.slice(i, i + FEATURES_PER_REQUEST));

featuresToRequest is used only for populating featureChunks, so it is safe to modify.

Copilot · 2025-07-11T19:23:59Z

src/config-utils.ts

@@ -195,7 +196,7 @@ export interface AugmentationProperties {
  /**
   * Extra query exclusions to append to the config.
   */
-  extraQueryExclusions?: ExcludeQueryFilter[];
+  extraQueryExclusions: ExcludeQueryFilter[];


Changing extraQueryExclusions from optional to required is a breaking change to the interface. This could cause compilation errors for existing code that doesn't provide this property.

Suggested change

extraQueryExclusions: ExcludeQueryFilter[];

extraQueryExclusions?: ExcludeQueryFilter[];

All uses of extraQueryExclusions are in this repository, and they have all been checked and updated. Furthermore, AugmentationProperties is never deserialized from files, so there is no concern for compatibility with earlier versions.

Copilot · 2025-07-11T19:23:59Z

src/feature-flags.ts

-      const remoteFlags = response.data as GitHubFeatureFlagsApiResponse;
+        .map(([f]) => f);
+
+      const FEATURES_PER_REQUEST = 25;


The magic number 25 should be defined as a named constant at the module level to improve maintainability and make it easier to adjust if API limits change.

Suggested change

const FEATURES_PER_REQUEST = 25;

const FEATURES_PER_REQUEST = FEATURES_PER_REQUEST_LIMIT;

nickrolfe · 2025-07-14T13:05:14Z

src/config-utils.ts

+    const feature = OVERLAY_ANALYSIS_FEATURES[language];
+    if (feature && (await features.getValue(feature, codeql))) {
+      continue;
+    }
+    const codeScanningFeature =
+      OVERLAY_ANALYSIS_CODE_SCANNING_FEATURES[language];
+    if (
+      codeScanningFeature &&
+      (await features.getValue(codeScanningFeature, codeql))
+    ) {
+      enableForCodeScanningOnly = true;
+      continue;
+    }
+    return false;


If I'm reading this correctly, then we won't check the code-scanning variant for a language if the main feature flag for that language is enabled. If, for example, the overlay_analysis_java flag is already enabled, then toggling overlay_analysis_code_scanning_java will have no effect. That seems reasonable, but we should take care that anyone enabling feature flags is aware of this.

nickrolfe · 2025-07-14T13:34:54Z

src/feature-flags.ts

+      for (const chunk of featureChunks) {
+        const response = await getApiClient().request(
+          "GET /repos/:owner/:repo/code-scanning/codeql-action/features",
+          {
+            owner: this.repositoryNwo.owner,
+            repo: this.repositoryNwo.repo,
+            features: chunk.join(","),
+          },
+        );
+        const chunkFlags = response.data as GitHubFeatureFlagsApiResponse;
+        remoteFlags = { ...remoteFlags, ...chunkFlags };
+      }


This chunking seems like the riskiest part of the PR, since it could affect all uses of the Action. I don't see anything wrong with it, and I see you've added a test (with some API-mocking) for it, but do we have a way to see it working in practice before merging/deploying?

All the PR checks run with the changes in this PR, so any outright failure would show up on CI checks.

For reference, the commit without request chunking that I first pushed to this PR had 244 check failures. It was hard to miss.

In addition to passing CI checks, I reran one of the workflows with debug logs. In the "Initialize CodeQL" step, we can see in the debug logs that the action was able to obtain status for all declared features (under "Loaded the following default values for the feature flags from the Code Scanning API"), which lends further support that the request chunking is working as intended.

I am also wondering what the performance impact of this is (both for users and the backend). If I follow our implementation correctly, we always fetch the values for all FFs that the Action knows of. So that means, once merged, the Action will fetch the status of all of these FFs for every run for everyone. I imagine it's fine if it's one request with a few FFs, but if this now spans multiple requests for every CodeQL analysis, that could quickly add up?

So that means, once merged, the Action will fetch the status of all of these FFs for every run for everyone.

The Action fetches the status of all declared Features for every run for everyone. This PR does not change that behavior—that will continue to be the case even if we do not merge this PR.

What this PR changes is to support fetching feature status in batches of 25 in accordance with the API limit. In practical terms, where "fetching the status of all declared features" used to require 1 API call, now it requires 2 API calls. Each call takes less than 1 second on my laptop, so adding one more call should not have any perceivable impact on user experience. With the volume of Code Scanning runs, I don't think it will have perceivable impact on backend load either.

The Action fetches the status of all declared Features for every run for everyone. This PR does not change that behavior—that will continue to be the case even if we do not merge this PR.

Sorry for the ambiguity in what I wrote. What I meant with "all these FFs" was "all the new FFs introduced by this PR". I am happy for you to keep this as is as long as you're happy that doubling the number of requests to this endpoint isn't a problem.

cklin · 2025-07-14T16:35:28Z

@mbg I'd also appreciate your review and approval on this PR before I merge.

mbg

LGTM. Just one minor suggestion for the test.

mbg · 2025-07-16T12:10:19Z

src/feature-flags.test.ts

+    // We only need to call getValue once, and it does not matter which feature
+    // we ask for. Under the hood, the features library will request all features
+    // from the API.
+    const feature = Feature.OverlayAnalysis;


Given that Feature elements are intended to be removed when the feature has fully shipped, we would need to update this test when OverlayAnalysis is removed. How about something like this instead:

Suggested change

const feature = Feature.OverlayAnalysis;

const feature = Object.values(Feature)[0];

However, I see that we similarly pick specific elements elsewhere in the tests (and that they have to be updated when they are removed). It's your choice if you want to fix this in this case or let us deal with it more generally.

cklin added 11 commits July 9, 2025 14:32

Extract loadUserConfig()

f4358b3

Use exclude-from-incremental also for overlay analysis

64fce58

Overlay database mode tests: list features

4bdb7fe

Before we introduce additional features for controlling overlay analysis enablement, change the unit tests to specify features directly instead of through a isFeatureEnabled boolean field.

Extract isOverlayAnalysisFeatureEnabled()

55cb6b8

Overlay: define language-specific features

9f70a5f

Overlay: check per-language features

d799ff5

Extract generateCodeScanningConfig()

fb77176

Overlay: check code-scanning features

90d7727

Move initializeFeatures() to testing-utils

b37e7e2

This change eliminates the need for setup-codeql.test to import from feature-flags.test, which makes the former run all tests defined in the latter.

cklin force-pushed the cklin/overlay-feature-flags branch from 1ab4d58 to bb8638d Compare July 11, 2025 17:04

cklin added 3 commits July 11, 2025 10:44

Replicate "too many feature flags" error in test

f939cb8

Limit Code Scanning API to 25 features per request

62e44ae

build: refresh js files

f3fa160

cklin force-pushed the cklin/overlay-feature-flags branch from bb8638d to f3fa160 Compare July 11, 2025 19:07

cklin marked this pull request as ready for review July 11, 2025 19:23

Copilot AI review requested due to automatic review settings July 11, 2025 19:23

cklin requested a review from a team as a code owner July 11, 2025 19:23

cklin requested review from nickrolfe and mbg July 11, 2025 19:23

Copilot AI reviewed Jul 11, 2025

View reviewed changes

nickrolfe approved these changes Jul 14, 2025

View reviewed changes

Fireskull81 approved these changes Jul 14, 2025

View reviewed changes

mbg approved these changes Jul 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Overlay: additional feature flags #2967

Overlay: additional feature flags #2967

cklin commented Jul 10, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 11, 2025

Uh oh!

cklin Jul 11, 2025

Uh oh!

Copilot AI Jul 11, 2025

Uh oh!

cklin Jul 11, 2025

Uh oh!

Copilot AI Jul 11, 2025

Uh oh!

nickrolfe Jul 14, 2025

Uh oh!

nickrolfe Jul 14, 2025

Uh oh!

cklin Jul 14, 2025

Uh oh!

mbg Jul 14, 2025

Uh oh!

cklin Jul 14, 2025

Uh oh!

mbg Jul 16, 2025

Uh oh!

cklin commented Jul 14, 2025

Uh oh!

mbg left a comment

Uh oh!

mbg Jul 16, 2025

Uh oh!

Uh oh!

		while (featuresToRequest.length > 0) {
		featureChunks.push(featuresToRequest.splice(0, FEATURES_PER_REQUEST));

	extraQueryExclusions: ExcludeQueryFilter[];
	extraQueryExclusions?: ExcludeQueryFilter[];

	const FEATURES_PER_REQUEST = 25;
	const FEATURES_PER_REQUEST = FEATURES_PER_REQUEST_LIMIT;

	const feature = Feature.OverlayAnalysis;
	const feature = Object.values(Feature)[0];

Overlay: additional feature flags #2967

Are you sure you want to change the base?

Overlay: additional feature flags #2967

Conversation

cklin commented Jul 10, 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 Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 11, 2025

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 Jul 14, 2025

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!