Add test and fix failures around loading shell environment on Windows by niik · Pull Request #21621 · desktop/desktop

niik · 2026-02-13T12:50:33Z

Closes #[issue number]

Description

This started out as a PR to just add a get-shell-env test but ended up causing a few changes after problems discovered. I'll try to document these in the diff

Screenshots

Release notes

Notes:

Add "cwd": "${workspaceFolder}" and change the test globals and env file to relative paths (./app/test/globals.mts and .test.env). This makes the VSCode test launch configuration more portable and ensures paths resolve correctly when running tests from the integrated terminal while keeping ${relativeFile} for targeting the current test file.

So turns out the Git resolved via PATH might point to the ming64 version which is fine but we need to deal with it. Dealt with that. Second, that unearthed a bug that findGitBashInRegistry returns the path to the git-bash.exe, not bash.exe which we need. Deal with that too

Prevent Node from messing up our perfectly quoted args

This was introduced in #8150 but it's causing our get-shell-env tests to fail because powershell v1 requires USERPROFILE to be set. I don't know what local gitconfig values were being overridden that caused this to be a problem in the first place but I think we should remove these and find out and fix it some other way if it's still a problem

niik · 2026-02-13T15:39:13Z

.vscode/launch.json

        "--import",
        "tsx",
        "--import",
-        "${workspaceFolder}/app/test/globals.mts",


The debug current test file script was broken in Windows because absolute pats need to be specified with the file:// scheme and I don't know how to do that so instead I made all paths relative and set the cwd

niik · 2026-02-13T15:39:29Z

app/src/lib/hooks/get-shell-env.ts

  cwd?: string,
-  shellKind?: SupportedHooksEnvShell
+  shellKind?: SupportedHooksEnvShell,
+  printenvzPath?: string


This just lets us override the path in tests

niik · 2026-02-13T15:40:12Z

app/src/lib/hooks/get-shell.ts

+  let bashPath = null
+  if (gitPath.toLowerCase().endsWith('\\cmd\\git.exe')) {
+    bashPath = join(gitPath, '../../usr/bin/bash.exe')
+  } else if (gitPath.toLowerCase().endsWith('\\mingw64\\bin\\git.exe')) {


So on my VM which(git) resolved to the mingw64 path. Don't know why it didn't do that when I was testing this out previously but now we handle that scenario as well

niik · 2026-02-13T15:40:53Z

app/src/lib/hooks/get-shell.ts

+  } else if (gitPath.toLowerCase().endsWith('\\mingw64\\bin\\git.exe')) {
+    bashPath = join(gitPath, '../../../usr/bin/bash.exe')
+  } else {
+    const HKLM = HKEY.HKEY_LOCAL_MACHINE


Previously we were using findGitBashInRegisty but that resolves the git-bash.exe which will always launch a window vs bash.exe which is what we want to invoke so I rolled my own here

niik · 2026-02-13T15:41:45Z

app/src/lib/hooks/get-shell.ts

    COMSPEC && /^(?:.*\\)?cmd(?:\.exe)?$/i.test(COMSPEC) ? COMSPEC : 'cmd.exe'
  const { args, quoteCommand } = cmd
-  return { shell, args, quoteCommand }
+  return { shell, args, quoteCommand, windowsVerbatimArguments: true }


This is the default behavior for node when spawning with shell: true and shell resolving to cmd.exe. We need it here as well or else node will re-quote our already quoted arg

niik · 2026-02-13T15:42:13Z

.test.env

 GIT_COMMITTER_NAME = 'Joe Bloggs'
 GIT_COMMITTER_EMAIL = 'joe.bloggs@somewhere.com'
 TEST_ENV = '1'
-HOME = ''


This was introduced in #8150 but it's causing our get-shell-env tests to fail because powershell v1 requires USERPROFILE to be set. I don't know what local gitconfig values were being overridden that caused this to be a problem in the first place but I think we should remove these and find out and fix it some other way if it's still a problem

Copilot

Pull request overview

Adds unit coverage for getShellEnv and adjusts Windows shell/env loading behavior to address failures when spawning shells to read environment variables.

Changes:

Added a unit test validating that getShellEnv returns an environment containing PATH.
Updated Windows shell resolution (Git Bash discovery + cmd.exe spawn options) and made getShellEnv accept an optional printenvz path for tests.
Tweaked VS Code test debugging configuration and simplified .test.env.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
app/test/unit/get-shell-env-test.ts	New unit test exercising `getShellEnv` across shells (Windows) / default shell (non-Windows).
app/src/lib/hooks/get-shell.ts	Adjusts Git Bash discovery logic and `cmd.exe` spawn settings for Windows.
app/src/lib/hooks/get-shell-env.ts	Adds optional `printenvzPath` parameter to support unit testing.
.vscode/launch.json	Makes debug test launch paths relative to workspace root.
.test.env	Removes some environment overrides previously set for test debugging.

app/src/lib/hooks/get-shell.ts

app/test/unit/get-shell-env-test.ts

app/src/lib/hooks/get-shell.ts

sergiou87 · 2026-02-16T15:47:16Z

app/src/lib/hooks/get-shell.ts

+  if (gitPath.toLowerCase().endsWith('\\cmd\\git.exe')) {
+    bashPath = join(gitPath, '../../usr/bin/bash.exe')
+  } else if (gitPath.toLowerCase().endsWith('\\mingw64\\bin\\git.exe')) {
+    bashPath = join(gitPath, '../../../usr/bin/bash.exe')


I'm always confused by this: some paths use \\ some other /. Is this correct? or should we use join(gitPath, '..', '..', '..', 'usr', 'bin', 'bash.exe') ?

Yeah, this is safe because Node runs normalize on the path after joining so those / will become \. See https://nodejs.org/docs/latest/api/path.html#pathjoinpaths

joins all given path segments together using the platform-specific separator as a delimiter, then normalizes the resulting path

And https://nodejs.org/docs/latest/api/path.html#pathnormalizepath

Since Windows recognizes multiple path separators, both separators will be replaced by instances of the Windows preferred separator ():

path.win32.normalize('C:////temp\\\\/\\/\\/foo/bar'); // Returns: 'C:\\temp\\foo\\bar'

Add an explicit TypeScript type for bashPath in app/src/lib/hooks/get-shell.ts (string | null) to improve type safety and satisfy strict typing rules. This is a non-functional change — no runtime behavior is altered.

niik added 5 commits February 13, 2026 13:49

Add smoke test for getShellEnv

b18728d

We'll take it from here

fc37e00

Prevent Node from messing up our perfectly quoted args

niik changed the title ~~Add smoke test for getShellEnv~~ Add test and fix failures around loading shell environment on Windows Feb 13, 2026

niik commented Feb 13, 2026

View reviewed changes

niik marked this pull request as ready for review February 13, 2026 15:46

niik requested a review from Copilot February 16, 2026 15:11

Copilot started reviewing on behalf of niik February 16, 2026 15:12 View session

Copilot AI reviewed Feb 16, 2026

View reviewed changes

app/src/lib/hooks/get-shell.ts Outdated Show resolved Hide resolved

app/test/unit/get-shell-env-test.ts Show resolved Hide resolved

app/src/lib/hooks/get-shell.ts Outdated Show resolved Hide resolved

sergiou87 reviewed Feb 16, 2026

View reviewed changes

sergiou87 previously approved these changes Feb 16, 2026

View reviewed changes

niik added 2 commits February 17, 2026 09:34

Fall back to registry if which(git) fails

56a47d7

Type annotate bashPath as string | null

96c4ae4

Add an explicit TypeScript type for bashPath in app/src/lib/hooks/get-shell.ts (string | null) to improve type safety and satisfy strict typing rules. This is a non-functional change — no runtime behavior is altered.

niik dismissed sergiou87’s stale review via 96c4ae4 February 17, 2026 08:35

sergiou87 approved these changes Feb 17, 2026

View reviewed changes

niik merged commit 41f1328 into development Feb 17, 2026
7 checks passed

niik deleted the add-get-shell-env-smoke-test branch February 17, 2026 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test and fix failures around loading shell environment on Windows#21621

Add test and fix failures around loading shell environment on Windows#21621
niik merged 7 commits intodevelopmentfrom
add-get-shell-env-smoke-test

niik commented Feb 13, 2026 •

edited

Loading

Uh oh!

niik Feb 13, 2026

Uh oh!

niik Feb 13, 2026

Uh oh!

niik Feb 13, 2026

Uh oh!

niik Feb 13, 2026

Uh oh!

niik Feb 13, 2026

Uh oh!

niik Feb 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sergiou87 Feb 16, 2026

Uh oh!

niik Feb 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

niik commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

niik commented Feb 13, 2026 •

edited

Loading