Skip to content

fix(vscode): include process path env variable in passed path#4770

Merged
benfdking merged 1 commit intomainfrom
fix_for_env
Jun 19, 2025
Merged

fix(vscode): include process path env variable in passed path#4770
benfdking merged 1 commit intomainfrom
fix_for_env

Conversation

@benfdking
Copy link
Copy Markdown
Contributor

@benfdking benfdking commented Jun 19, 2025

  • For venvs, we override the PATH env variable, whereas we should be pretending it with the value rather than overriding.

@benfdking benfdking requested a review from Copilot June 19, 2025 21:15
Copy link
Copy Markdown
Contributor

Copilot AI left a 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 ensures that the interpreter’s bin directory is prepended to the system PATH when invoking SQLMesh commands, so any required binaries are resolvable at runtime.

  • Prepend interpreterDetails.binPath to the existing PATH instead of overriding it
  • Applied this change in three execution helpers: getTcloudBin, sqlmeshExec, and sqlmeshLspExec
Comments suppressed due to low confidence (1)

vscode/extension/src/utilities/sqlmesh/sqlmesh.ts:86

  • There are no existing tests validating that the PATH string correctly prepends the interpreter bin directory to the original PATH. Consider adding a unit test for the helper (once extracted) to assert this behavior.
      PATH: `${interpreterDetails.binPath!}${path.delimiter}${process.env.PATH || ''}`,

Comment thread vscode/extension/src/utilities/sqlmesh/sqlmesh.ts
Comment thread vscode/extension/src/utilities/sqlmesh/sqlmesh.ts Outdated
@benfdking benfdking enabled auto-merge (squash) June 19, 2025 21:18
@benfdking benfdking merged commit 17a7098 into main Jun 19, 2025
24 checks passed
@benfdking benfdking deleted the fix_for_env branch June 19, 2025 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants