Skip to content

fix: resolve shell tabs via OS user shell#58

Open
ASRagab wants to merge 4 commits intojohannesjo:mainfrom
ASRagab:fix/resolve-user-shell-selection
Open

fix: resolve shell tabs via OS user shell#58
ASRagab wants to merge 4 commits intojohannesjo:mainfrom
ASRagab:fix/resolve-user-shell-selection

Conversation

@ASRagab
Copy link
Copy Markdown
Contributor

@ASRagab ASRagab commented Apr 5, 2026

Summary

  • resolve default shell selection via os.userInfo().shell before falling back to process.env.SHELL
  • use the shared resolver for both PTY shell tabs and PATH bootstrap in the Electron main process
  • add focused tests covering OS shell, env fallback, and /bin/sh fallback behavior

Why

Parallel Code currently falls back to the inherited SHELL environment variable when no explicit shell command is provided. That can cause shell tabs to launch bash even when the user's actual account login shell is zsh (or another shell), depending on how Electron was launched.

Using the OS-reported user shell first makes the default shell selection more faithful and predictable.

Test Plan

  • npm run typecheck
  • npm test

Copy link
Copy Markdown

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 improves default shell selection in the Electron main process by resolving the user’s login shell from the OS (via os.userInfo().shell) before falling back to process.env.SHELL, and reuses the same resolver for both PTY shell tabs and PATH bootstrapping.

Changes:

  • Add a shared resolveUserShell() helper with OS-first, env fallback, then /bin/sh (or cmd.exe) fallback behavior.
  • Use resolveUserShell() in fixPath() (Electron main) and PTY shell tab spawning.
  • Add Vitest coverage for OS shell preference and fallback scenarios.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
electron/user-shell.ts Introduces shared shell-resolution logic (OS shell → env SHELL → platform default).
electron/user-shell.test.ts Adds tests validating preference and fallback behavior for shell resolution.
electron/main.ts Switches PATH bootstrap to use the shared shell resolver.
electron/ipc/pty.ts Switches PTY default shell command to use the shared shell resolver.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +19 to +23
try {
const osShell = normalizeShell(userInfo().shell);
if (osShell) return osShell;
} catch {
// Fall back to inherited environment if the OS lookup is unavailable.
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

resolveUserShell() returns the OS-reported shell path without verifying it exists/is executable. If a user's account shell is misconfigured (or points to a removed binary), this will now cause shell tabs to fail (via validateCommand) even if env.SHELL or /bin/sh would work. Consider checking executability before returning osShell, and fall back to env.SHELL//bin/sh when the OS shell isn't runnable.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,30 @@
import * as os from 'node:os';
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

For consistency with the rest of the Electron code (e.g., electron/main.ts and electron/ipc/pty.ts), consider importing the Node built-in as import * as os from 'os' instead of using the node:-prefixed specifier.

Suggested change
import * as os from 'node:os';
import * as os from 'os';

Copilot uses AI. Check for mistakes.
@ASRagab
Copy link
Copy Markdown
Contributor Author

ASRagab commented Apr 9, 2026

@johannesjo welcome back. Updated.

@johannesjo
Copy link
Copy Markdown
Owner

Review notes (Copilot's two comments appear already addressed in the current version — canUseShell/isExecutablePosixShell added, import uses 'os' not 'node:os'.)

Two new items worth considering:

1. Unrelated .prettierignore changes

The diff includes two lines added to .prettierignore:

CLAUDE.local.md
.enzyme/

These appear to be local developer config that crept in. They have no relation to the shell-resolution fix and should be dropped from this PR.

2. Missing test for shell: null from os.userInfo()

normalizeShell(null) correctly returns null, and the code handles it, but there is no explicit test covering the case where userInfo() returns shell: null (as opposed to shell: '' which is tested). The os.userInfo() typings allow shell to be null on some platforms, so a test like the empty-string case but with null would close that gap.

No correctness bugs found. The core logic (os.userInfo().shellSHELL env → /bin/sh) is sound, edge cases (throws, empty, non-executable) are handled and tested, and the DI-based testability design is clean.

@johannesjo
Copy link
Copy Markdown
Owner

Thank you very much! <3

Copy link
Copy Markdown
Owner

@johannesjo johannesjo left a comment

Choose a reason for hiding this comment

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

The resolveUserShell() implementation is correct and the tests are comprehensive. One minor scope issue: the diff includes .prettierignore entries that appear unrelated to the shell-resolution fix — please check if these were accidentally included from another branch and clean up the diff. Once that's resolved this is ready to merge. Details in the review comment.

@ASRagab
Copy link
Copy Markdown
Contributor Author

ASRagab commented Apr 14, 2026

removed entries...thanks

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