Skip to content

fix(vscode): pick the print environment command by shell - #6091

Open
tripleaceme wants to merge 3 commits into
SQLMesh:mainfrom
tripleaceme:fix/shell-aware-print-env
Open

tripleaceme wants to merge 3 commits into
SQLMesh:mainfrom
tripleaceme:fix/shell-aware-print-env

Conversation

@tripleaceme

Copy link
Copy Markdown
Contributor

Description

Closes #5608.

sqlmesh: Print Environment opens a terminal and sends a command to list the environment. It chose that command by platform, treating "not Windows" as "POSIX shell with env":

if (IS_WINDOWS) {
  terminal.sendText('set')
} else {
  terminal.sendText('env | sort')
}

The command is now chosen from the shell that is actually in use. The decision lives in a pure function so it can be tested without a VS Code mock, and printEnvironment.ts is one call into it.

shell command
fish set
bash / zsh / sh / dash / ksh / ash, and anything unrecognized on a non-Windows host `env
cmd set
PowerShell / pwsh Get-ChildItem Env: | Sort-Object Name

Unrecognized shells fall back on the platform default, so nothing that works today stops working.

Two things worth knowing, both covered on the issue

The reporter's premise is not quite right. fish has real pipes and env and sort are ordinary binaries there, so env | sort does print the environment correctly in fish today. This is an idiomatic-output improvement, not a crash fix — set additionally lists fish's own shell variables in fish's own formatting, which is what a fish user expects.

The genuine breakage is on Windows, and the issue does not mention it. In PowerShell, set is an alias for Set-Variable, which with no arguments does not print the environment — it prompts Supply values for the following parameters: Name[0]: and waits. PowerShell is VS Code's default terminal on Windows, so this command has been unusable for most Windows users. Fixed here because it is the same decision, but it does widen the change beyond the issue's text; happy to split it out if you would rather.

I should be clear about confidence on that: it follows from Set-Variable's documented behaviour rather than from a run. I have neither Windows nor fish available, so neither end of this is verified in a live shell, and the PowerShell replacement command is unverified too.

Why vscode.env.shell

It is stable API since 1.38 and this extension targets ^1.96.0. Its documentation says it is the detected default shell overridden by terminal.integrated.defaultProfile, so reading that setting directly would be strictly worse — it holds a profile name, not a path, and does not resolve the auto-detect case. process.env.SHELL is the extension host's login shell, which is not necessarily what VS Code launches, and is absent on Windows. Decisively, this command calls createTerminal with no shellPath, so the terminal it opens is exactly the one vscode.env.shell describes.

Test Plan

13 tests in src/utilities/shellCommand.test.ts, covering fish (including a Homebrew path), the POSIX family, cmd and PowerShell with and without .exe, a POSIX shell on Windows (Git Bash — POSIX wins over platform), a bare name with no path, mixed case, an unknown shell, and no shell reported at all (vscode.env.shell is '' where none exists).

Rather than reverting once, I checked the tests against three separate mutants, because reverting to the original behaviour leaves some tests green for the right reason:

mutant result
original isWindows ? 'set' : 'env | sort' 7 failed, 6 passed
always set 10 failed, 3 passed
cmd mapped to a wrong value 2 failed

Union across the three: every one of the 13 tests fails under at least one mutant, so none is passing vacuously.

pnpm run lint                      # ui-style: prettier + eslint + tsc, all workspaces
pnpm run ci  (vscode/extension)    # test-vscode: eslint, tsc --noEmit, vitest
20 tests passed (3 files)

Nothing exercises printEnvironment.ts itself, since there is no VS Code mock in this repo and I did not add one; the tests pin the decision function only.

Checklist

  • I have run make style and fixed any issues
  • I have added tests for my changes (if applicable)
  • All existing tests pass (make fast-test)
  • My commits are signed off (git commit -s) per the DCO

The print environment command opened a terminal with the user's default
shell and then branched only on the platform, sending `set` on Windows
and `env | sort` everywhere else. That assumes every non-Windows shell is
a POSIX shell and every Windows shell is cmd.

Resolve `vscode.env.shell` to a shell family instead and send the command
that family accepts: `set` for fish, `env | sort` for POSIX shells,
`set` for cmd and `Get-ChildItem Env: | Sort-Object Name` for PowerShell.
`set` in PowerShell is an alias for `Set-Variable`, which prompts for a
variable name rather than printing the environment, so the previous
command did not work in the default Windows shell.

An unrecognised shell falls back on the platform command, which keeps the
previous behaviour for any shell not listed.

Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
@tripleaceme

Copy link
Copy Markdown
Contributor Author

@cmgoffena13 — closes #5608. This is the option 1 I asked about on the issue (detect the shell); the description explains why I would argue for option 2 now that it is built, and the PR is shaped so switching is a deletion of one file and one import.

The part worth your eye is not fish: set in PowerShell does not print the environment, it prompts for a variable name. PowerShell is VS Code's default terminal on Windows, so that branch has been broken for most Windows users. Fixed here since it is the same decision, but it does widen the change beyond the issue.

@cmgoffena13 cmgoffena13 self-assigned this Oct 5, 2026

@cmgoffena13 cmgoffena13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small adjustments, comments added. Reminder that any new files need a // SPDX-License-Identifier: Apache-2.0 header to align to our contributing standards.

* shell family.
*/
const PRINT_ENVIRONMENT_COMMANDS: Record<ShellFamily, string> = {
fish: 'set',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be set -x so it only lists exported variables

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d239db3. Agreed — a bare set in fish also lists shell-local variables, so it was printing more than the other families do. set -x is the exported-only listing, which is what env gives on the POSIX side.

*/
const PRINT_ENVIRONMENT_COMMANDS: Record<ShellFamily, string> = {
fish: 'set',
powershell: 'Get-ChildItem Env: | Sort-Object Name',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will truncate values, we should have it be: Get-ChildItem Env: | Sort-Object Name | ForEach-Object { '{0}={1}' -f $_.Name, $_.Value }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d239db3, using your exact command. The default table view truncating at console width is the kind of thing that only shows up on a machine with long values, so good catch — and formatting each one as NAME=value also lines the output up with what env | sort prints.

@@ -0,0 +1,94 @@
import { describe, it, expect } from 'vitest'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add in another test to that if we have pwsh and isWindows false, it still returns the powershell command correctly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in d239db3 as should use the PowerShell command for pwsh off Windows, covering both /usr/local/bin/pwsh and a bare pwsh.

I checked it isn't a test that passes no matter what. The regression it guards is PowerShell being treated as Windows-only, so I gated the lookup behind !(family === 'powershell' && !isWindows) and confirmed the new test fails, then restored it and confirmed all 14 pass. Worth noting the command-layer coverage was the gap — detectShellFamily('pwsh', false) was already asserted, but nothing checked that the command came back right off Windows.

fish's bare `set` also lists shell-local variables, so it is now `set -x`,
which matches what the other families print.

PowerShell's default table view truncates values at the console width, so
each variable is now formatted explicitly as `NAME=value`.

Adds the pwsh-off-Windows case for printEnvironmentCommand, and the SPDX
headers both new files were missing.

Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
@tripleaceme

Copy link
Copy Markdown
Contributor Author

All three comments addressed in d239db3, replied inline. The SPDX headers were a miss on my part — both new files now carry // SPDX-License-Identifier: Apache-2.0, matching coalesceAsync.ts. printEnvironment.ts is a modification rather than a new file, so I left it alone; say the word if you want it added there too.

eslint, prettier, tsc and the extension unit suite are all clean (33 tests).

One caveat on the two command changes: neither fish nor PowerShell is installed on this machine, so I applied both on the reasoning rather than by running them. set -x as fish's exported-only listing and the console-width truncation in PowerShell's default table view both check out against the documented behaviour, but I haven't put either command in front of a real shell.

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.

The VS Code SQLMesh extension assumes I am using bash or zsh

2 participants