-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -38,9 +38,11 @@ import type { VariantAnalysisResultsManager } from "./variant-analysis-results-m | |||||
| import { | ||||||
| getAutofixPath, | ||||||
| getAutofixModel, | ||||||
| getAutofixCapiDevKey, | ||||||
| downloadTimeout, | ||||||
| AUTOFIX_PATH, | ||||||
| AUTOFIX_MODEL, | ||||||
| AUTOFIX_CAPI_DEV_KEY, | ||||||
| } from "../config"; | ||||||
| import { asError, getErrorMessage } from "../common/helpers-pure"; | ||||||
| import { createTimeoutSignal } from "../common/fetch-stream"; | ||||||
|
|
@@ -155,6 +157,39 @@ async function findLocalAutofix(): Promise<string> { | |||||
| return localAutofixPath; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Finds and resolves the Copilot API dev key from the `codeQL.autofix.capiDevKey` setting. | ||||||
| * The key can be specified as an environment variable reference (e.g., `env:MY_ENV_VAR`) | ||||||
| * or a 1Password secret reference (e.g., `op://vault/item/field`). By default, it uses | ||||||
| * the environment variable `CAPI_DEV_KEY`. | ||||||
| * | ||||||
| * @returns The resolved Copilot API dev key. | ||||||
| * @throws Error if the Copilot API dev key is not found or invalid. | ||||||
| */ | ||||||
| async function findCapiDevKey(): Promise<string> { | ||||||
| let capiDevKey = getAutofixCapiDevKey() || "env:CAPI_DEV_KEY"; | ||||||
|
|
||||||
| if (!capiDevKey.startsWith("env:") && !capiDevKey.startsWith("op://")) { | ||||||
| // Don't allow literal keys in config.json for security reasons | ||||||
| throw new Error( | ||||||
| `Invalid CAPI dev key format. Use 'env:<ENV_VAR_NAME>' or 'op://<1PASSWORD_SECRET_REFERENCE>'.`, | ||||||
| ); | ||||||
| } | ||||||
| if (capiDevKey.startsWith("env:")) { | ||||||
| const envVarName = capiDevKey.substring("env:".length); | ||||||
| capiDevKey = process.env[envVarName] || ""; | ||||||
| } | ||||||
| if (capiDevKey.startsWith("op://")) { | ||||||
| capiDevKey = await opRead(capiDevKey); | ||||||
| } | ||||||
| if (!capiDevKey) { | ||||||
| throw new Error( | ||||||
| `Copilot API dev key not found. Make sure ${AUTOFIX_CAPI_DEV_KEY.qualifiedName} is set correctly.`, | ||||||
|
||||||
| `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.`, |
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.
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),capiDevKeybecomes an empty string, but then line 182 checks if it starts withop://, which will be false. The empty string should trigger the error at line 185-188, but the control flow is confusing. Consider usingelse ifat line 182 to make the mutually exclusive nature of these options clearer and prevent potential confusion.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.