Use per-language CodeQL bundles - #4146
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Copilot review overview
🟡 Changes recommended
Fallback telemetry underreports total setup duration by excluding the failed per-language request.
Get a fresh assessment by requesting another Copilot review.
Review tier: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds feature-gated per-language CodeQL bundle selection, extraction, fallback, telemetry, and validation.
Changes:
- Selects eligible release/nightly per-language bundles with 404 fallback.
- Keeps partial bundles out of the toolcache and reports telemetry.
- Adds unit and generated end-to-end validation across ten languages.
| File | Description |
|---|---|
src/per-language-bundles.ts |
Implements bundle eligibility and URL recognition. |
src/per-language-bundles.test.ts |
Tests eligibility and recognition. |
src/setup-codeql.ts |
Resolves, downloads, caches, and falls back between bundles. |
src/setup-codeql.test.ts |
Tests selection, fallback, versions, and caching. |
src/tools-download.ts |
Extends download telemetry. |
src/status-report.ts |
Defines telemetry fields. |
src/init-action.ts |
Reports init telemetry. |
src/setup-codeql-action.ts |
Reports setup telemetry. |
src/feature-flags.ts |
Adds the disabled-by-default feature flag. |
pr-checks/sync.ts |
Supports custom generated matrices. |
pr-checks/checks/per-language-bundle-validation.yml |
Validates all supported language bundles. |
pr-checks/checks/bundle-toolcache.yml |
Preserves combined-bundle cache testing. |
.github/workflows/codescanning-config-cli.yml |
Ensures reusable combined-bundle caching. |
lib/entry-points.js |
Generated artifact; content excluded. |
.github/workflows/__bundle-toolcache.yml |
Generated workflow; content excluded. |
.github/workflows/__per-language-bundle-validation.yml |
Generated workflow; content excluded. |
Files excluded by content exclusion policy (3)
- .github/workflows/__bundle-toolcache.yml
- .github/workflows/__per-language-bundle-validation.yml
- lib/entry-points.js
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mbg
left a comment
There was a problem hiding this comment.
I have had a look through the changes here and added quite a few detailed comments. Additionally, some high-level ones:
- Like with the other PR this is stacked on, this was a lot more difficult (and time-consuming) to review than it needed to be, because all of the changes were contained in one commit (except the follow-up to Copilot's review comment). There were clear opportunities for breaking the changes up and structuring them to be more incremental.
- Quite a few of the function-level comments and test names make sense in the context of this PR where it's obvious that they relate to per-language vs combined bundles, but wouldn't make sense outside of this context. Some of my review comments highlight specific cases of this, but I didn't comment on all of them.
- Try and remember to make use of the new
ActionStateinfrastructure whenever possible in new or changed code. In particular, thegetPerLanguageBundleLanguagefunction would benefit from this since the tests would not need to beserial.
| if (!isGitHubHostedRunner()) { | ||
| // Per-language installs stay out of the toolcache; self-hosted runners should retain | ||
| // the reusable combined bundle instead. | ||
| return explain("the job is not running on a GitHub-hosted runner"); | ||
| } |
There was a problem hiding this comment.
I am wondering if this is the correct choice. Self-hosted runners could use custom images / persistent storage to pre-install the desired CLI bundle in the toolcache. If that's not available, wouldn't it be more desirable to download the language-specific bundle for improved performance like on GH-hosted runners?
There was a problem hiding this comment.
Self-hosted runner can indeed preinstall CodeQL, but I don't think we should assume it. I would prefer to have a better way to determine whether the toolcache is persistent or not before rolling out to self-hosted runners to avoid a situation where we install many per-language bundles and use an unnecessary amount of disk space.
| const client = github.getOctokit("123", { | ||
| request: { | ||
| fetch: async () => | ||
| new Response(JSON.stringify([{ tag_name: tagName }]), { | ||
| headers: { "content-type": "application/json" }, | ||
| }), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
This is less precise than the stubbing that stubHostedNightly replaces. Consider whether to stub sinon.stub(client.rest.repos, "listReleases"); instead of request.
There was a problem hiding this comment.
As far as I can tell, this hasn't been addressed, but that's not blocking.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Lots of good ideas, thank you for the detailed review! |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mbg
left a comment
There was a problem hiding this comment.
Thanks for tackling all the review comments I left during the previous round! I have reviewed the new commits up to and including 2f552a9. I'll look at the remaining ones later this evening, but figured it would be good to post what I have so far now in the interest of time.
Most of the comments I added so far are minor, but there are a couple where I think the changes haven't quite been what I intended with my comments.
As a high-level point, I appreciate that the feedback is addressed across multiple commits, but the breakdown is still not ideal. Most of the new commits I have reviewed so far have a description that suggests they address one point of feedback, but actually mix in other smaller changes as well (some of which address other feedback and some of which are unrelated to the previous review). That makes it harder to verify whether a particular piece of feedback was addressed because more context switching is required while reviewing the individual commits and it takes a bit more effort to identify whether a given change relates to the main point of feedback or is unrelated.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mbg
left a comment
There was a problem hiding this comment.
I have now reviewed the remaining commits from before my last review, as well as the ones in which you addressed my feedback from yesterday. Thanks for taking care of all that!
There are a few very minor points that are left, none of which are blocking / all of which can be followed-up on later. I am happy to approve this PR in this state and I'll go ahead and merge it.
| return databaseBundlePath; | ||
| } | ||
|
|
||
| /** Returns the elapsed milliseconds, rounded, since a `performance.now()` timestamp. */ |
There was a problem hiding this comment.
Minor: "since a performance.now() timestamp" is incomplete
| const language = await getPerLanguageBundleLanguage( | ||
| { | ||
| initAllState({ | ||
| env: getTestEnv(), |
There was a problem hiding this comment.
Minor: not required if it's not customised. initAllState will automatically call getTestEnv() to initialise env.
| initAllState({ | ||
| env, | ||
| features: createFeatures(enabledFeatures), | ||
| logger: getRecordingLogger([], { logToConsole: false }), |
There was a problem hiding this comment.
Minor: Not needed if you don't inspect the log output. initAllState will automatically initialise a logger.
|
|
||
| // Check whether per-language bundles are published for the requested CLI version. | ||
| // Skip this for the latest nightly, whose tag contains a date rather than a CLI version. | ||
| // Latest-nightly selection skips this release-version check, but not the other eligibility checks. |
There was a problem hiding this comment.
Minor: The comment now again doesn't explain why it's OK to skip.
| // Latest-nightly selection skips this release-version check, but not the other eligibility checks. | |
| // If `nightly` tools were requested, skip the version check since we don't have one at this point. |
| const client = github.getOctokit("123", { | ||
| request: { | ||
| fetch: async () => | ||
| new Response(JSON.stringify([{ tag_name: tagName }]), { | ||
| headers: { "content-type": "application/json" }, | ||
| }), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
As far as I can tell, this hasn't been addressed, but that's not blocking.

Download self-contained per-language bundles for eligible analyses on GitHub.com. Selection requires one explicitly configured language, a GitHub-hosted runner, zstd, and a supported language/platform combination. Release bundles require CodeQL 2.27.1 or newer; nightlies use the same eligibility rules without the release-version check. The
per_language_bundlesfeature flag remains off by default.Extract per-language bundles into the runner's temporary directory rather than the toolcache. If an automatically selected asset returns 404, fall back to the combined bundle from the same release. Explicit per-language
toolsURLs are not substituted.Add bundle-language and fallback telemetry, plus generated nightly PR checks for all ten languages covering extractor contents, toolcache isolation, database creation and analysis. Update existing cache-dependent checks to request multiple languages so they continue to use the combined bundle.
Risk assessment
Low risk: Automatic per-language bundle selection is feature-flagged and off by default. The unflagged behavior is limited to rare, explicitly supplied per-language bundle URLs and prevents those partial installs from entering the toolcache.
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
per_language_bundles.How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist