| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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
Bundle eligibility is consistently propagated and covered by focused tests without unresolved correctness issues.
Review effort: Balanced
Findings: None
Updates bundle selection to use combined bundles whenever configured queries may depend on other languages’ library packs.
Changes:
| 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. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.
Sorry, something went wrong.
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>
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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
I agree that we are more conservative than necessary, which is fine, and that it would make sense to look into refactoring this as future work.
Sorry, something went wrong.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…fig` input Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… check Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…age-pr-check-failures # Conflicts: # lib/entry-points.js Co-authored-by: henrymercer <14129055+henrymercer@users.noreply.github.com>
There was a problem hiding this comment.
A bunch more comments on this, sorry. There's nothing critical, but just a bunch of instances where comments are worded poorly or odd changes were made as part of a refactoring.
Sorry, something went wrong.
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>
…is known to send 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>
…pper Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…put test Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…isely Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Thank you! I think all of my feedback is addressed now and I don't have anything to add on the most recent set of commits.
Happy to approve on that basis. I haven't re-reviewed the full changes. If there's anything that you'd like me to take another look over, let me know. Otherwise, feel free to go ahead and merge this!
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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 config input is unset or only sets threat models and model packs like default setup's does, and the queries input and github-codeql-extra-queries repository 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. The config input is parsed once, before setting up CodeQL, and used both for this check and to configure the analysis. As a result, an invalid config input now fails before CodeQL is downloaded, with an error that names the config input rather than a temporary file.
This PR also modifies setup-codeql to 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 its languages and analysis-kinds inputs, which said to also pass them to init, even though init fails if setup-codeql has 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 defaultSuites so that per-language-bundles.ts can use it without an import cycle.
Finally, it moves per-language bundles to a new per_language_bundles_v2 feature 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_bundles or per_language_bundles_v2 feature flag is enabled.
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?
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