From 63816043cf690351fe412584c7828f8bbe65b84c Mon Sep 17 00:00:00 2001 From: Test User Date: Wed, 31 Dec 2025 19:06:13 -0500 Subject: [PATCH] feat: enhance shell detection logic and improve cross-platform support - Updated the TerminalService to utilize getShellPaths() for better shell detection across platforms. - Improved logic for detecting user-configured shells in WSL and added fallbacks for various platforms. - Enhanced unit tests to mock shell paths for comprehensive cross-platform testing, ensuring accurate shell detection behavior. These changes aim to streamline shell detection and improve the user experience across different operating systems. --- apps/server/src/services/terminal-service.ts | 127 ++++++++++++------ .../unit/services/terminal-service.test.ts | 40 +++++- libs/platform/src/system-paths.ts | 38 +++++- 3 files changed, 154 insertions(+), 51 deletions(-) diff --git a/apps/server/src/services/terminal-service.ts b/apps/server/src/services/terminal-service.ts index 06d56981..81a1585a 100644 --- a/apps/server/src/services/terminal-service.ts +++ b/apps/server/src/services/terminal-service.ts @@ -14,7 +14,12 @@ import * as path from 'path'; import * as secureFs from '../lib/secure-fs.js'; // System paths module handles shell binary checks and WSL detection // These are system paths outside ALLOWED_ROOT_DIRECTORY, centralized for security auditing -import { systemPathExists, systemPathReadFileSync, getWslVersionPath } from '@automaker/platform'; +import { + systemPathExists, + systemPathReadFileSync, + getWslVersionPath, + getShellPaths, +} from '@automaker/platform'; // Maximum scrollback buffer size (characters) const MAX_SCROLLBACK_SIZE = 50000; // ~50KB per terminal @@ -65,60 +70,96 @@ export class TerminalService extends EventEmitter { /** * Detect the best shell for the current platform + * Uses getShellPaths() to iterate through allowed shell paths */ detectShell(): { shell: string; args: string[] } { const platform = os.platform(); + const shellPaths = getShellPaths(); - // Check if running in WSL + // Helper to get basename handling both path separators + const getBasename = (shellPath: string): string => { + const lastSep = Math.max(shellPath.lastIndexOf('/'), shellPath.lastIndexOf('\\')); + return lastSep >= 0 ? shellPath.slice(lastSep + 1) : shellPath; + }; + + // Helper to get shell args based on shell name + const getShellArgs = (shell: string): string[] => { + const shellName = getBasename(shell).toLowerCase().replace('.exe', ''); + // PowerShell and cmd don't need --login + if (shellName === 'powershell' || shellName === 'pwsh' || shellName === 'cmd') { + return []; + } + // sh doesn't support --login in all implementations + if (shellName === 'sh') { + return []; + } + // bash, zsh, and other POSIX shells support --login + return ['--login']; + }; + + // Check if running in WSL - prefer user's shell or bash with --login if (platform === 'linux' && this.isWSL()) { - // In WSL, prefer the user's configured shell or bash - const userShell = process.env.SHELL || '/bin/bash'; - if (systemPathExists(userShell)) { - return { shell: userShell, args: ['--login'] }; + const userShell = process.env.SHELL; + if (userShell) { + // Try to find userShell in allowed paths + for (const allowedShell of shellPaths) { + if (allowedShell === userShell || getBasename(allowedShell) === getBasename(userShell)) { + try { + if (systemPathExists(allowedShell)) { + return { shell: allowedShell, args: getShellArgs(allowedShell) }; + } + } catch { + // Path not allowed, continue searching + } + } + } + } + // Fall back to first available POSIX shell + for (const shell of shellPaths) { + try { + if (systemPathExists(shell)) { + return { shell, args: getShellArgs(shell) }; + } + } catch { + // Path not allowed, continue + } } return { shell: '/bin/bash', args: ['--login'] }; } - switch (platform) { - case 'win32': { - // Windows: prefer PowerShell, fall back to cmd - const pwsh = 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe'; - const pwshCore = 'C:\\Program Files\\PowerShell\\7\\pwsh.exe'; - - if (systemPathExists(pwshCore)) { - return { shell: pwshCore, args: [] }; + // For all platforms: first try user's shell if set + const userShell = process.env.SHELL; + if (userShell && platform !== 'win32') { + // Try to find userShell in allowed paths + for (const allowedShell of shellPaths) { + if (allowedShell === userShell || getBasename(allowedShell) === getBasename(userShell)) { + try { + if (systemPathExists(allowedShell)) { + return { shell: allowedShell, args: getShellArgs(allowedShell) }; + } + } catch { + // Path not allowed, continue searching + } } - if (systemPathExists(pwsh)) { - return { shell: pwsh, args: [] }; - } - return { shell: 'cmd.exe', args: [] }; - } - - case 'darwin': { - // macOS: prefer user's shell, then zsh, then bash - const userShell = process.env.SHELL; - if (userShell && systemPathExists(userShell)) { - return { shell: userShell, args: ['--login'] }; - } - if (systemPathExists('/bin/zsh')) { - return { shell: '/bin/zsh', args: ['--login'] }; - } - return { shell: '/bin/bash', args: ['--login'] }; - } - - case 'linux': - default: { - // Linux: prefer user's shell, then bash, then sh - const userShell = process.env.SHELL; - if (userShell && systemPathExists(userShell)) { - return { shell: userShell, args: ['--login'] }; - } - if (systemPathExists('/bin/bash')) { - return { shell: '/bin/bash', args: ['--login'] }; - } - return { shell: '/bin/sh', args: [] }; } } + + // Iterate through allowed shell paths and return first existing one + for (const shell of shellPaths) { + try { + if (systemPathExists(shell)) { + return { shell, args: getShellArgs(shell) }; + } + } catch { + // Path not allowed or doesn't exist, continue to next + } + } + + // Ultimate fallbacks based on platform + if (platform === 'win32') { + return { shell: 'cmd.exe', args: [] }; + } + return { shell: '/bin/sh', args: [] }; } /** diff --git a/apps/server/tests/unit/services/terminal-service.test.ts b/apps/server/tests/unit/services/terminal-service.test.ts index ca90937d..88660f7f 100644 --- a/apps/server/tests/unit/services/terminal-service.test.ts +++ b/apps/server/tests/unit/services/terminal-service.test.ts @@ -14,6 +14,7 @@ vi.mock('@automaker/platform', async () => { systemPathExists: vi.fn(), systemPathReadFileSync: vi.fn(), getWslVersionPath: vi.fn(), + getShellPaths: vi.fn(), // Mock shell paths for cross-platform testing isAllowedSystemPath: vi.fn(() => true), // Allow all paths in tests }; }); @@ -23,6 +24,36 @@ describe('terminal-service.ts', () => { let service: TerminalService; let mockPtyProcess: any; + // Shell paths for each platform (matching system-paths.ts) + const linuxShellPaths = [ + '/bin/zsh', + '/bin/bash', + '/bin/sh', + '/usr/bin/zsh', + '/usr/bin/bash', + '/usr/bin/sh', + '/usr/local/bin/zsh', + '/usr/local/bin/bash', + '/opt/homebrew/bin/zsh', + '/opt/homebrew/bin/bash', + 'zsh', + 'bash', + 'sh', + ]; + + const windowsShellPaths = [ + 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', + 'C:\\Program Files\\PowerShell\\7-preview\\pwsh.exe', + 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', + 'C:\\Windows\\System32\\cmd.exe', + 'pwsh.exe', + 'pwsh', + 'powershell.exe', + 'powershell', + 'cmd.exe', + 'cmd', + ]; + beforeEach(() => { vi.clearAllMocks(); service = new TerminalService(); @@ -45,6 +76,7 @@ describe('terminal-service.ts', () => { vi.mocked(platform.systemPathExists).mockReturnValue(true); vi.mocked(platform.systemPathReadFileSync).mockReturnValue(''); vi.mocked(platform.getWslVersionPath).mockReturnValue('/proc/version'); + vi.mocked(platform.getShellPaths).mockReturnValue(linuxShellPaths); // Default to Linux paths vi.mocked(secureFs.stat).mockResolvedValue({ isDirectory: () => true } as any); }); @@ -55,6 +87,7 @@ describe('terminal-service.ts', () => { describe('detectShell', () => { it('should detect PowerShell Core on Windows when available', () => { vi.mocked(os.platform).mockReturnValue('win32'); + vi.mocked(platform.getShellPaths).mockReturnValue(windowsShellPaths); vi.mocked(platform.systemPathExists).mockImplementation((path: string) => { return path === 'C:\\Program Files\\PowerShell\\7\\pwsh.exe'; }); @@ -67,6 +100,7 @@ describe('terminal-service.ts', () => { it('should fall back to PowerShell on Windows if Core not available', () => { vi.mocked(os.platform).mockReturnValue('win32'); + vi.mocked(platform.getShellPaths).mockReturnValue(windowsShellPaths); vi.mocked(platform.systemPathExists).mockImplementation((path: string) => { return path === 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe'; }); @@ -79,6 +113,7 @@ describe('terminal-service.ts', () => { it('should fall back to cmd.exe on Windows if no PowerShell', () => { vi.mocked(os.platform).mockReturnValue('win32'); + vi.mocked(platform.getShellPaths).mockReturnValue(windowsShellPaths); vi.mocked(platform.systemPathExists).mockReturnValue(false); const result = service.detectShell(); @@ -114,7 +149,10 @@ describe('terminal-service.ts', () => { it('should fall back to bash on macOS if zsh not available', () => { vi.mocked(os.platform).mockReturnValue('darwin'); vi.spyOn(process, 'env', 'get').mockReturnValue({}); - vi.mocked(platform.systemPathExists).mockReturnValue(false); + // zsh not available, but bash is + vi.mocked(platform.systemPathExists).mockImplementation((path: string) => { + return path === '/bin/bash'; + }); const result = service.detectShell(); diff --git a/libs/platform/src/system-paths.ts b/libs/platform/src/system-paths.ts index 95fa4b24..2824d623 100644 --- a/libs/platform/src/system-paths.ts +++ b/libs/platform/src/system-paths.ts @@ -109,21 +109,45 @@ export function getClaudeProjectsDir(): string { /** * Get common shell paths for shell detection + * Includes both full paths and short names to match $SHELL or PATH entries */ export function getShellPaths(): string[] { if (process.platform === 'win32') { return [ - process.env.COMSPEC || 'cmd.exe', - 'cmd.exe', - 'powershell.exe', - 'pwsh.exe', // PowerShell Core short form - 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', + // Full paths (most specific first) 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', - 'C:\\Program Files\\PowerShell\\7-preview\\pwsh.exe', // Preview versions + 'C:\\Program Files\\PowerShell\\7-preview\\pwsh.exe', + 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', + // COMSPEC environment variable (typically cmd.exe) + process.env.COMSPEC || 'C:\\Windows\\System32\\cmd.exe', + // Short names (for PATH resolution) + 'pwsh.exe', + 'pwsh', + 'powershell.exe', + 'powershell', + 'cmd.exe', + 'cmd', ]; } - return ['/bin/zsh', '/bin/bash', '/bin/sh', '/usr/bin/zsh', '/usr/bin/bash']; + // POSIX (macOS, Linux) + return [ + // Full paths + '/bin/zsh', + '/bin/bash', + '/bin/sh', + '/usr/bin/zsh', + '/usr/bin/bash', + '/usr/bin/sh', + '/usr/local/bin/zsh', + '/usr/local/bin/bash', + '/opt/homebrew/bin/zsh', + '/opt/homebrew/bin/bash', + // Short names (for PATH resolution or $SHELL matching) + 'zsh', + 'bash', + 'sh', + ]; } // =============================================================================