GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Do not use `core.exportVariable` in unit tests by mbg · Pull Request #3930 · github/codeql-action

Conversation

@mbg

@mbg mbg commented May 22, 2026

Copy link
Copy Markdown
Member

Although we already clear environment variables that are set during unit tests, that does not affect the behaviour of core.exportVariable which additionally sets environment variables for subsequent steps in a workflow. If the unit tests are run in CI, then core.exportVariable sets 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.exportVariable which does not call core.exportVariable when NODE_ENV is test (which is set automatically by ava).

Risk assessment

For internal use only. Please select the risk level of this change:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Environments:

  • Testing/None - This change does not impact any CodeQL workflows in production.

How did/will you validate this change?

  • Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
  • End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

  • Development/testing only - This change cannot cause any failures in production.

How will you know if something goes wrong after this change is released?

  • Telemetry - I rely on existing telemetry or have made changes to the telemetry.

Are there any special considerations for merging or releasing this change?

  • No special considerations - This change can be merged at any time.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mbg added 3 commits May 22, 2026 13:45
@mbg
Move isInTestMode to environment.ts
ffebdc8
@mbg
Add wrapper around core.exportVariable
dc5f2b9
@mbg
Add basic eslint enforcement
d3b3ffb
@mbg mbg self-assigned this May 22, 2026
Copilot AI review requested due to automatic review settings May 22, 2026 14:04
@mbg
mbg requested a review from a team as a code owner May 22, 2026 14:04
@github-actions github-actions Bot added the size/M Should be of average difficulty to review label May 22, 2026
Copilot started reviewing on behalf of mbg May 22, 2026 14:05
View session
Copilot AI reviewed May 22, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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 in src/environment.ts and moved isInTestMode() there (with util re-export for compatibility).
  • Replaced many core.exportVariable(...) call sites across the action with the new wrapper.
  • Added an ESLint rule to prohibit core.exportVariable outside src/environment.ts, and updated affected unit tests to stub the wrapper.
Show a summary per file
File Description
src/environment.ts Adds isInTestMode and the exportVariable wrapper around core.exportVariable.
src/util.ts Switches env-var exports to the wrapper and re-exports isInTestMode.
src/upload-lib.ts Uses wrapper for sentinel env vars and deprecation warning env var.
src/status-report.ts Uses wrapper for job-status/workflow-start/test-environment env vars.
src/setup-codeql-action.ts Uses wrapper for job UUID and “has run” env vars.
src/init.ts Uses wrapper for the file-coverage deprecation sentinel env var.
src/init-action.ts Uses wrapper for job UUID, feature toggles, and forwarded env vars.
src/init-action-post.ts Uses wrapper to set final JOB_STATUS.
src/autobuild.ts Uses wrapper for autobuild feature env vars and Go autobuild sentinel.
src/autobuild-action.ts Uses wrapper for autobuild success sentinel.
src/api-client.ts Uses wrapper for persisting computed ANALYSIS_KEY.
src/analyze-action.ts Uses wrapper for SARIF output dir and analyze success sentinel.
src/debug-artifacts.ts Uses wrapper for artifact scan sentinel in test mode.
src/codeql.ts Uses wrapper for suppressing “deprecated soon” warning env var.
src/config-utils.ts Uses wrapper for TRAP caching env var.
src/overlay/caching.test.ts Updates stubbing to target environment.isInTestMode (instead of util).
src/init.test.ts Updates stubbing to target environment.exportVariable (instead of core.exportVariable).
eslint.config.mjs Adds a no-restricted-syntax rule to ban core.exportVariable usage outside src/environment.ts.
lib/entry-points.js Generated build output reflecting the TypeScript changes (not reviewed).

Copilot's findings

  • Files reviewed: 18/19 changed files
  • Comments generated: 1

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mbg reacted with thumbs down emoji
All reactions
  • 👎 1 reaction
Comment thread src/environment.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
github-advanced-security[bot]
github-advanced-security Bot found potential problems May 22, 2026
View reviewed changes
Comment thread src/environment.ts Fixed

Uh oh!

There was an error while loading. Please reload this page.
@mbg
Add NODE_ENV as safe environment variable
8041026
oscarsj
oscarsj reviewed May 22, 2026
View reviewed changes
Comment thread src/environment.ts
* 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") {

@oscarsj oscarsj May 22, 2026

Copy link
Copy Markdown
Member

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.

Shouldn't this just use the isInTestMode() function declared earlier? why the difference?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mbg mbg May 22, 2026

Copy link
Copy Markdown
Member Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mbg mbg mentioned this pull request May 28, 2026
Change waitForProcessing to use exponential backoff #3937
Merged
@mbg mbg mentioned this pull request Oct 7, 2026
Use setup-codeql logic for finding release assets in start-proxy #4198
Merged
@mbg
mbg marked this pull request as draft October 7, 2026 17:59
@mbg mbg mentioned this pull request Oct 7, 2026
Do not use core.exportVariable in unit tests (2nd attempt) #4212
Merged
@mbg

mbg commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

Closing this in favour of #4212
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mbg mbg closed this Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oscarsj oscarsj oscarsj left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
+1 more reviewer
@github-advanced-security github-advanced-security[bot] github-advanced-security[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@mbg mbg

Labels

size/M Should be of average difficulty to review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@mbg @oscarsj @github-advanced-security
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.