Use the combined bundle when queries may need other languages' library packs - #4184
henrymercer wants to merge 14 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…rary packs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…p-codeql` 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
🟢 Approval recommended
Bundle eligibility is consistently propagated and covered by focused tests without unresolved correctness issues.
Review effort: Balanced
Findings: None
What changed in this PR
Updates bundle selection to use combined bundles whenever configured queries may depend on other languages’ library packs.
Changes:
- Detects query configurations requiring combined bundles.
- Propagates bundle-selection reasoning through CodeQL setup.
- Updates tests, documentation, and PR-check configuration.
| File | Description |
|---|---|
src/per-language-bundles.ts |
Adds query eligibility logic. |
src/per-language-bundles.test.ts |
Tests eligibility and explanations. |
src/setup-codeql.ts |
Applies eligibility to release and nightly bundles. |
src/setup-codeql.test.ts |
Tests combined-bundle selection. |
src/setup-codeql-action.ts |
Forces combined bundles for standalone setup. |
src/init-action.ts |
Collects query configuration inputs. |
src/init.ts |
Propagates bundle-selection reasoning. |
src/codeql.ts |
Propagates setup options. |
src/codeql.test.ts |
Updates setup calls. |
src/upload-lib.ts |
Updates initialization call. |
src/config/db-config.ts |
Centralizes built-in suite names. |
src/analyze.ts |
Uses centralized suite names. |
src/analyze.test.ts |
Updates suite import. |
setup-codeql/action.yml |
Corrects input documentation. |
pr-checks/checks/export-file-baseline-information.yml |
Disables per-language bundles for the baseline check. |
lib/entry-points.js |
Generated output; excluded from review. |
.github/workflows/__export-file-baseline-information.yml |
Generated workflow; excluded from review. |
Files excluded by content exclusion policy (2)
- .github/workflows/__export-file-baseline-information.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>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mbg
left a comment
There was a problem hiding this comment.
Thank you for taking care of this! I think this approach makes sense to work around the issue. Query customisation is reasonably rare so that this still allows most users to benefit from per-language bundles, while avoiding any issues like we saw in CI for those that do customise them.
I left a few comments, with one or two points about long-term maintainability and otherwise minor comments. So, generally this looks pretty good already and it shouldn't be far off from being ready to merge.
| gitHubVersion.type, | ||
| codeQLDefaultVersionInfo, | ||
| rawLanguages, | ||
| undefined, // otherLanguagePacksReason: this Action doesn't take a query configuration |
There was a problem hiding this comment.
Minor: Prefer placing the comment above the relevant line, rather than at the end of it.
There was a problem hiding this comment.
I feel the other way — I'll leave this as is, but open to discussing this in general and updating the linter configuration.
| */ | ||
| export function getOtherLanguagePacksReason( | ||
| inputs: QueryConfigInputs, | ||
| ): string | undefined { |
There was a problem hiding this comment.
Minor: Consider creating an enum for the reasons, returning a value of that enum here, and then mapping to a string when needed.
There was a problem hiding this comment.
I can see some value in listing all the reasons, but since we also have values associated with the reasons (e.g. the config file path), I don't listing the reasons is worth the complexity.
| if (inputs.configInput !== undefined && !inputs.isDynamicWorkflow) { | ||
| return "the 'config' input may use queries that need library packs for other languages"; | ||
| } |
There was a problem hiding this comment.
Long-term issue: if our hard-coded assumption about dynamic workflows ever changes in any of the products that use dynamic workflows, then this will be quite tricky to track down. Perhaps to guard a little again that and be extra defensive, consider introducing a constant somewhere along the lines of ASSUME_DEFAULT_SETUP_CONFIG_NO_QUERIES with a suitable comment and factor that into the logic here.
There was a problem hiding this comment.
Are you thinking of adding an environment variable to the default setup template as an indicator of when we can no longer assume that the default setup config won't specify custom queries? Perhaps it would be simpler to indeed parse the config input and check that it only has the properties we expect?
There was a problem hiding this comment.
Perhaps it would be simpler to indeed parse the config input and check that it only has the properties we expect?
I've implemented this.
There was a problem hiding this comment.
That seems like a better approach, thanks!
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ry property 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>
…cked Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…lows 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.
Thank you for addressing the comments from my previous review! I've had some more comments on the changes here. The main ones are again related to the maintainability of duplicated logic and whether we can reduce that or move some of the work to a common place before any of the consumers.
| * Parses the `queries` input, a comma-separated list of queries that's combined with the queries | ||
| * from the configuration if it starts with '+'. The `input` of the result is `undefined` if the | ||
| * value is unset or empty. Entries aren't validated, so an empty entry becomes `{ uses: "" }`. |
There was a problem hiding this comment.
Minor: This comment is largely about how this function is used, not what it does. I.e. this function is completely disconnected from the queries input and doesn't control how the returned combines value is interpreted by the caller.
| * Parses the `github-codeql-extra-queries` repository property, which has the same format as the | ||
| * `queries` input. The `input` of the result is `undefined` if the value is unset or empty. Entries | ||
| * aren't validated, so an empty entry becomes `{ uses: "" }`. |
There was a problem hiding this comment.
Minor: Same issue with this comment as with the one for parseQueriesInput.
| export function parseExtraQueriesProperty( | ||
| value: string | undefined, | ||
| ): Augmentation<QuerySpec[]> { |
There was a problem hiding this comment.
This function is essentially the same as parseQueriesInput, except that it provides the additional argument to parseQueriesFromInput. When I added the repo property, I added the extra, optional parameter to parseQueriesFromInput so that the function could be shared between the two uses.
Consider whether parseQueriesInput could have a second, optional parameter for the error like parseQueriesFromInput does to avoid having a mostly duplicate parseExtraQueriesProperty or whether we could move the shouldCombine call into parseQueriesFromInput.
| const query = findNonBuiltInQuery( | ||
| parseQueriesInput(inputs.queriesInput).input, | ||
| ); |
There was a problem hiding this comment.
Thanks for changing this to better reuse the existing implementation!
Another concern here, which I don't think is blocking for this PR, is that getOtherLanguagePacksReason is more conservative than it needs to be. Specifically, we ignore the combines property and so are ignoring the precedence / combination rules that are implemented in combineQueries. Since that may discard some of the configured queries if others take precedence / don't allow combining, not all of the configured queries may actually end up getting passed to the CLI. If that is the case, then we might disable per-language bundles here even though we may not need to. Since this is more conservative than necessary, it is fine as-is, but we could look into reusing more of the combineQueries logic here to ensure consistency and to allow per-language bundles in more cases.
| * @throws A `ConfigurationError` if the `queries` input or the `github-codeql-extra-queries` | ||
| * repository property is a '+' with no queries after it, unless an input that's checked earlier | ||
| * already gives a reason. |
There was a problem hiding this comment.
Observation: getOtherLanguagePacksReason will now throw if parseQueriesInput or parseExtraQueriesProperty throws. Previously, getOtherLanguagePacksReason would never have thrown an error.
It's probably fine because the circumstances under which an error is thrown here would lead to an error later on in any case, but it moves that error to an earlier point in the execution.
I don't think that's an issue because I don't think there's anything happening in-between the two throwing sites that would particularly matter, but if we wanted to be more conservative then we could catch the exceptions in getOtherLanguagePacksReason and treat them as equivalent to no custom queries being configured in the respective case.
| // queries. For example, default setup only uses it for threat models and model packs. | ||
| // The `config` input can configure queries in the same way as a configuration file. We assume | ||
| // that dynamic workflows, which GitHub manages, don't use it to add queries. For example, default | ||
| // setup only uses it for threat models and model packs. |
There was a problem hiding this comment.
Super minor: instead of the example at the end, we could mention the DefaultSetupConfig type here (from db-config.ts) to look at for what assumptions we do make about the configuration provided by Default Setup. Since that affects the behaviour of the Action, we are more likely to update it if things change than the example in this comment.
| * uses, which are threat models and model packs, to valid values. Returns `false` otherwise, | ||
| * including if `contents` isn't valid YAML. | ||
| */ | ||
| export function matchesDefaultSetupConfigSchema(contents: string): boolean { |
There was a problem hiding this comment.
matchesDefaultSetupConfigSchema implements similar logic to mergeDefaultSetupAndUserConfigs with some minor differences (mergeDefaultSetupAndUserConfigs expects to already be given a UserConfig object and doesn't parse the YAML itself; both handle the outcomes of checkSchema differently).
Two concerns with this:
- We may end up parsing and validating the config twice. That seems a bit unnecessary.
- If we change one for any reason, we should also change the other.
Alongside the similar points for getOtherLanguagePacksReason, I am starting to think that maybe we should refactor this more so that we perform the input processing once before getOtherLanguagePacksReason and initConfig are called, and then pass the results of the input processing to both. I appreciate that it's more work to do that than we'd like for this though, and I haven't checked if any of the relevant input processing in initConfig has a dependency on the CLI.
Could we at least modify the flow so that the config input is parsed as YAML earlier and the resulting object can be used here as well as in mergeDefaultSetupAndUserConfigs?
| } catch (error) { | ||
| if (error instanceof yaml.YAMLException) { | ||
| return false; | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
If we are keeping the YAML processing here, I think it would be good to add an inline comment here before the if (error instanceof ...) check to explain why we are happy to treat a YAMLException as indicative that the configuration wasn't provided by Default Setup. (Maybe also add a logger.debug call to log that case.)
We support custom configuration files that include packs for other languages, for example:
as taken from our PR checks.
If these custom queries live in compiled packs, then the packs ship their own dependencies. However if the custom query is just a path to a QL file, then the dependencies are resolved from a bundle.
This creates an issue with per-language bundles: CodeQL resolves every configured query before selecting the ones for the analyzed language, so a single-language analysis with a configuration like the above will fail. This is evidenced by failures in the "Go: Custom queries" and "Start proxy" PR checks when using per-language bundles.
This PR only selects a per-language bundle when the query configuration known before CodeQL is set up can't reference such queries: there's no configuration file, the
configinput is unset or only sets threat models and model packs like default setup's does, and thequeriesinput andgithub-codeql-extra-queriesrepository property only name built-in query suites. This avoids loading configuration files before setting up CodeQL, at the cost of using the combined bundle for configuration files that only use built-in queries.This PR also modifies
setup-codeqlto always use the combined bundle, since it can't tell whether the queries that a workflow runs with the CLI will need library packs for other languages. A separate commit corrects the docs for itslanguagesandanalysis-kindsinputs, which said to also pass them toinit, even thoughinitfails ifsetup-codeqlhas run in the same job.It also disables per-language bundles in the "Export file baseline information" PR check, since a per-language bundle only reports file baseline information for its own language. The second commit moves
defaultSuitesso thatper-language-bundles.tscan use it without an import cycle.Finally, it moves per-language bundles to a new
per_language_bundles_v2feature flag, so that we can roll them out to Action versions that include this fix without also enabling them for earlier versions.Risk assessment
Low risk: The change only affects bundle selection when the
per_language_bundlesorper_language_bundles_v2feature flag is enabled.Which use cases does this change impact?
Workflow types:
configinput, or queries that aren't built-in query suites, and workflows that pass a single language tosetup-codeql.Products:
Environments:
How did/will you validate this change?
setup-codeqlAction's entry point has no unit tests, so its reason is only covered indirectly.If something goes wrong after this change is released, what are the mitigation and rollback strategies?
per_language_bundles_v2.How will you know if something goes wrong after this change is released?
initfailures in analyses that use a per-language bundle.Are there any special considerations for merging or releasing this change?
Merge / deployment checklist