Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a CodeQL Action–local wrapper for setting environment variables so unit tests avoid calling @actions/core.exportVariable, which can leak environment changes to subsequent CI steps. It then migrates internal calls over to the wrapper and adds an ESLint guardrail to prevent reintroducing direct core.exportVariable usage.
Changes:
- Added
exportVariable()wrapper insrc/environment.tsand movedisInTestMode()there (withutilre-export for compatibility). - Replaced many
core.exportVariable(...)call sites across the action with the new wrapper. - Added an ESLint rule to prohibit
core.exportVariableoutsidesrc/environment.ts, and updated affected unit tests to stub the wrapper.
Show a summary per file
Copilot's findings
- Files reviewed: 18/19 changed files
- Comments generated: 1
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * sets environment variables for other steps in a workflow when we run unit tests in CI. | ||
| */ | ||
| export function exportVariable(name: string, val: any): void { | ||
| if (process.env["NODE_ENV"] === "test") { |
There was a problem hiding this comment.
Shouldn't this just use the isInTestMode() function declared earlier? why the difference?
There was a problem hiding this comment.
isInTestMode is based on the CODEQL_ACTION_TEST_MODE environment variable, which we primarily set for e2e tests. Historically, it was only about whether or not we should upload SARIF results, but has been used for some other purposes in that context.
The CODEQL_ACTION_TEST_MODE environment variable is not set for the unit tests. I did initially have a version of exportVariable that was based on isInTestMode before I noticed that. However, rather than trying to decide if it would be OK to set CODEQL_ACTION_TEST_MODE for the unit tests or not (since it changes the behaviour), I figured that testing whether we are running the unit tests or not is a safer approach that involves less potential for change.
Although we already clear environment variables that are set during unit tests, that does not affect the behaviour of
core.exportVariablewhich additionally sets environment variables for subsequent steps in a workflow. If the unit tests are run in CI, thencore.exportVariablesets environment variables for subsequent steps in the workflow job which can interfere with them.This PR improves the situation by introducing a wrapper around
core.exportVariablewhich does not callcore.exportVariablewhenNODE_ENVistest(which is set automatically byava).Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Environments:
How did/will you validate this change?
.test.tsfiles).pr-checks).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