| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Couple of questions inline.
Also, to answer yours, I think it's fine to make the feature flags request in init AND analyze.
What do you think about adding a comment of storing the feature flags in the config file as a potential future improvement?
Sorry, something went wrong.
There was a problem hiding this comment.
This is good to go in after updating the version range.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This PR builds on #856 to add support for running ML-powered queries. Currently these queries would only be run for customers (a) performing JavaScript analysis, (b) running the security-extended (or security-and-quality) query suite, and (c) whose repos are opted into the ml_powered_queries feature flag.
The action happens in src/config-utils.ts: when all of these conditions hold, we add the codeql/javascript-experimental-atm-queries pack to the analysis.
Since the security review for this feature is not yet complete, these queries will currently return no results. We will not roll out the feature flag until this security review is complete to avoid needlessly increasing the runtime of customers' analyses by downloading the ML-powered query pack.
Points for review
We want to enable ML-powered queries based in part on which query suite the customer is running. We resolve query suites into query paths in the init action, and the analysis action to my knowledge doesn't know what query suites the user requested. Therefore the init action is the natural point to test whether we should run ML-powered queries.
However this does necessitate an API call to test feature flag enablement in the init action as well as the existing one in the analysis action that's used for database upload.
Another option is saving some kind of information (e.g. feature flags or the query suites requested) to the config file in the init action and loading them in the analysis action. I didn't go with this since it seems like overengineering for the benefit of one fewer API call, and the API call in consideration doesn't count towards rate limits, but what do reviewers think?
Merge / deployment checklist