Add "View Autofixes" feature for variant analysis results #4065
Conversation
This function will increment file numbers when running autofix in a loop
jcogs33
force-pushed
the
jcogs33/view-autofixes
branch
3 times, most recently
from
8095989 to
c5f0e5e
Compare
jcogs33
force-pushed
the
jcogs33/view-autofixes
branch
from
c5f0e5e to
3ff5d64
Compare
extensions/ql-vscode/src/variant-analysis/variant-analysis-view.ts
Outdated
Show resolved
Hide resolved
extensions/ql-vscode/src/variant-analysis/variant-analysis-view.ts
Outdated
Show resolved
Hide resolved
extensions/ql-vscode/src/view/variant-analysis/VariantAnalysisActions.tsx
Outdated
Show resolved
Hide resolved
| env: { | ||
| CAPI_DEV_KEY: process.env.CAPI_DEV_KEY, | ||
| PATH: process.env.PATH, | ||
| }, |
There was a problem hiding this comment.
Would it make sense to allow specifying the environment variables in the config (in something like codeQL.autofix.env)?
There was a problem hiding this comment.
I'm not sure I'm understanding how to specify these in the config. i.e. Are you suggesting specifying the API key for CAPI_DEV_KEY directly in the settings.json file (is that safe)? Or specifying the path to a local file where the API key is stored? Or something else?
There was a problem hiding this comment.
I don't think we want to add the secret as a config setting. There are ways of adding secrets securely, but I think just keeping it as an env var is fine. Much simpler. Just please be sure to document.
There are some complications if you are using a codespace. I'm not sure how it would work. Can you inject env vars into it?
As for the PATH variable. I'm not sure if it makes sense either.
There was a problem hiding this comment.
I agree that we probably don't want to add the secret in the config file, but allowing you to set custom environment variables might still be useful, for example an ENVIRONMENT environment variable. We could also just pass through all of process.env to set custom environment variables though.
I'm not sure exactly how VS Code picks up environment variables, but it seems like it's possible to specify secrets when using Codespaces: Using secrets.
| `Invalid empty or undefined arguments: ${args.join(" ")}`, | ||
| ); | ||
| } | ||
| const p = spawn(bin, args, { stdio: [0, 1, 2], ...options }); |
There was a problem hiding this comment.
I think stdio: [0, 1, 2] will ensure that the autofix CLI logs to the stdout/stderr of VS Code, but I don't think you can view those. I think it would make sense to log those to the extension logs. A good example of this can be found in runCodeQlCliInNewProcess, specifically in handleProcessOutput.
|
|
||
| // Extract the tarball | ||
| await new Promise<void>((resolve, reject) => { | ||
| const process = spawn("tar", ["-xzf", tarballPath], { cwd: downloadDir }); |
There was a problem hiding this comment.
We're using tar-stream elsewhere for extracting tarballs. It would be better to use something like that instead of spawning a new process.
Just be aware that tar-stream is currently a dev dependency and would need to be moved to a dependency.
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Thanks for the reviews @koesie10 and @aeisenberg! 🙇 |
There was a problem hiding this comment.
There's one more comment from @koesie10 that should be addressed, but after that I think it's good to go.
@aeisenberg Which comment are you referring to? There were four things I still was planning to address: three comments related to editing parts of |
|
Oh...there might be more things that I missed. I'm out tomorrow. I think it's also a holiday in the US, so I can take another look on Monday. |
Description
Adds a "View Autofixes" feature for variant analysis results. This is a canary-only feature.
Generates autofixes for selected variant analysis results and opens the resulting autofix outputs in a markdown file.
Activated via the "View Autofixes" button in the variant analysis webview or via right-clicking the variant analysis query history item.
At a high-level, the implementation:
Commit-by-commit review recommended. Let me know if you want me to split anything onto a separate PR.
Consideration
queryHelpOverrideDirectoryto the newgo/pkg/autofix/prompt/qhelpsdirectory in that PR.Testing