-
Notifications
You must be signed in to change notification settings - Fork 218
MRVA: use Go Autofix instead of Cocofix #4215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
a11ebc0 to
c5972fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates the internal MRVA Autofix feature from using a JavaScript-based Cocofix binary to a Go-based autofix binary. It also introduces flexible API key configuration that supports environment variables and 1Password secret references.
Key changes:
- Added new configuration setting
codeQL.autofix.capiDevKeyfor API key management - Implemented secure API key resolution with support for environment variables and 1Password CLI
- Updated binary name from
cocofix.jstoautofixand adjusted related paths
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| extensions/ql-vscode/src/config.ts | Adds new capiDevKey setting and getter function for autofix configuration |
| extensions/ql-vscode/src/variant-analysis/view-autofixes.ts | Implements API key resolution logic, adds 1Password CLI integration, updates binary paths and names from Cocofix to Go autofix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if (capiDevKey.startsWith("op://")) { |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic allows a value that starts with env: but resolves to an empty string to fall through to the 1Password check. If the environment variable doesn't exist (line 180), capiDevKey becomes an empty string, but then line 182 checks if it starts with op://, which will be false. The empty string should trigger the error at line 185-188, but the control flow is confusing. Consider using else if at line 182 to make the mutually exclusive nature of these options clearer and prevent potential confusion.
| } | |
| if (capiDevKey.startsWith("op://")) { | |
| } else if (capiDevKey.startsWith("op://")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to allow the option of the environment variable containing an op:// reference.
| async function opRead(secretReference: string): Promise<string> { | ||
| return new Promise((resolve, reject) => { | ||
| const opProcess = spawn("op", ["read", secretReference], { | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }); |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the op command is not found on the PATH, the error from line 884-885 will be a system error like "ENOENT" which may not be clear to users. Consider catching this specific case and providing a more helpful error message indicating that the 1Password CLI needs to be installed and available on PATH.
| } | ||
| if (!capiDevKey) { | ||
| throw new Error( | ||
| `Copilot API dev key not found. Make sure ${AUTOFIX_CAPI_DEV_KEY.qualifiedName} is set correctly.`, |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message doesn't provide enough context about why the key wasn't found. Since the key could be missing due to an undefined environment variable or failed 1Password read, consider including which method was attempted (env var name or op:// reference) to help users debug the issue.
| `Copilot API dev key not found. Make sure ${AUTOFIX_CAPI_DEV_KEY.qualifiedName} is set correctly.`, | |
| `Copilot API dev key not found. Attempted to retrieve using '${getAutofixCapiDevKey() || "env:CAPI_DEV_KEY"}'. Make sure ${AUTOFIX_CAPI_DEV_KEY.qualifiedName} is set correctly and that the referenced environment variable or 1Password secret exists and is accessible.`, |
Updates the (still internal, therefore no change note) MRVA Autofix feature to use the latest Go-based binary.
Can be configured to use either an env var (
env:<ENV_VAR_NAME>) or a 1Password secret reference (op://<vault>/<item>/<field>; requiresopcommand) for the Copilot API key. By default, it uses theCAPI_DEV_KEYenv var, as before.�[31m- }.run();�[0m. Was it like that before?Example autofix output
mysql/mysql-connector-j
Fix suggestion details
Changes: Will patch: src/test/java/testsuite/regression/StatementRegressionTest.java ... } �[31m- }.run();�[0m �[32m+ }.start();�[0m Description: To fix this issue, replace the direct call to `run()` with a call to `start()`. The `start()` method initializes a new thread and then calls the `run()` method internally, ensuring that the code in `run()` executes asynchronously on a separate thread. This approach preserves the intended concurrency. Steps: 1. Locate the erroneous line (`.run()`). 2. Replace `.run()` with `.start()` to ensure the `Thread` runs in its own thread. 3. Validate that this change aligns with the intended behavior of the code. --- Fix is valid.Notes
chinabugotech/hutool
Fix suggestion details
Changes: Will patch: hutool-core/src/test/java/cn/hutool/core/io/WatchMonitorTest.java ... public void testDir() { monitor = WatchMonitor.createAll("d:/", new DelayWatcher(watcher, 500)); �[31m- monitor.run();�[0m �[32m+ monitor.start();�[0m } Description: To fix the problem, we should properly start the `WatchMonitor` instance on a new thread instead of calling its `run()` method directly. This typically involves using the `start()` method, which ensures the `run()` method executes on a separate thread as intended. In this case, the following changes are needed: 1. Replace `monitor.run()` with `monitor.start()` in the affected test methods: `testDir` and `testDelay`. 2. No other modifications appear necessary since the `monitor` object is already correctly set up. --- Fix is valid. Changes: Will patch: hutool-core/src/test/java/cn/hutool/core/io/WatchMonitorTest.java ... public void testDir() { monitor = WatchMonitor.createAll("d:/", new DelayWatcher(watcher, 500)); �[31m- monitor.run();�[0m �[32m+ new Thread(monitor).start();�[0m } Description: To fix this issue, we need to ensure that the `run()` method is called on a separate thread instead of the current thread. If `WatchMonitor` is a `Thread` or similar, then `start()` should be invoked instead of `run()`. If `WatchMonitor` is not directly a thread but implements `Runnable`, a `Thread` instance wrapping the `WatchMonitor` object should be created and started. This approach preserves the expected behavior of starting the monitoring task in a separate thread. There are two locations in the code where this issue appears: line 68 (`monitor.run()`) and line 84 (`monitor.run()`). Both should be modified to ensure a new thread is started. --- Fix is valid.Notes