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.
This commit is contained in:
Test User
2025-12-31 19:06:13 -05:00
parent eafe474dbc
commit 63816043cf
3 changed files with 154 additions and 51 deletions

View File

@@ -14,7 +14,12 @@ import * as path from 'path';
import * as secureFs from '../lib/secure-fs.js'; import * as secureFs from '../lib/secure-fs.js';
// System paths module handles shell binary checks and WSL detection // System paths module handles shell binary checks and WSL detection
// These are system paths outside ALLOWED_ROOT_DIRECTORY, centralized for security auditing // 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) // Maximum scrollback buffer size (characters)
const MAX_SCROLLBACK_SIZE = 50000; // ~50KB per terminal const MAX_SCROLLBACK_SIZE = 50000; // ~50KB per terminal
@@ -65,61 +70,97 @@ export class TerminalService extends EventEmitter {
/** /**
* Detect the best shell for the current platform * Detect the best shell for the current platform
* Uses getShellPaths() to iterate through allowed shell paths
*/ */
detectShell(): { shell: string; args: string[] } { detectShell(): { shell: string; args: string[] } {
const platform = os.platform(); 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()) { if (platform === 'linux' && this.isWSL()) {
// In WSL, prefer the user's configured shell or bash const userShell = process.env.SHELL;
const userShell = process.env.SHELL || '/bin/bash'; if (userShell) {
if (systemPathExists(userShell)) { // Try to find userShell in allowed paths
return { shell: userShell, args: ['--login'] }; 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'] }; return { shell: '/bin/bash', args: ['--login'] };
} }
switch (platform) { // For all platforms: first try user's shell if set
case 'win32': { const userShell = process.env.SHELL;
// Windows: prefer PowerShell, fall back to cmd if (userShell && platform !== 'win32') {
const pwsh = 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe'; // Try to find userShell in allowed paths
const pwshCore = 'C:\\Program Files\\PowerShell\\7\\pwsh.exe'; 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(pwshCore)) { // Iterate through allowed shell paths and return first existing one
return { shell: pwshCore, args: [] }; for (const shell of shellPaths) {
try {
if (systemPathExists(shell)) {
return { shell, args: getShellArgs(shell) };
} }
if (systemPathExists(pwsh)) { } catch {
return { shell: pwsh, args: [] }; // 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: '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: [] }; return { shell: '/bin/sh', args: [] };
} }
}
}
/** /**
* Detect if running inside WSL (Windows Subsystem for Linux) * Detect if running inside WSL (Windows Subsystem for Linux)

View File

@@ -14,6 +14,7 @@ vi.mock('@automaker/platform', async () => {
systemPathExists: vi.fn(), systemPathExists: vi.fn(),
systemPathReadFileSync: vi.fn(), systemPathReadFileSync: vi.fn(),
getWslVersionPath: vi.fn(), getWslVersionPath: vi.fn(),
getShellPaths: vi.fn(), // Mock shell paths for cross-platform testing
isAllowedSystemPath: vi.fn(() => true), // Allow all paths in tests isAllowedSystemPath: vi.fn(() => true), // Allow all paths in tests
}; };
}); });
@@ -23,6 +24,36 @@ describe('terminal-service.ts', () => {
let service: TerminalService; let service: TerminalService;
let mockPtyProcess: any; 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(() => { beforeEach(() => {
vi.clearAllMocks(); vi.clearAllMocks();
service = new TerminalService(); service = new TerminalService();
@@ -45,6 +76,7 @@ describe('terminal-service.ts', () => {
vi.mocked(platform.systemPathExists).mockReturnValue(true); vi.mocked(platform.systemPathExists).mockReturnValue(true);
vi.mocked(platform.systemPathReadFileSync).mockReturnValue(''); vi.mocked(platform.systemPathReadFileSync).mockReturnValue('');
vi.mocked(platform.getWslVersionPath).mockReturnValue('/proc/version'); 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); vi.mocked(secureFs.stat).mockResolvedValue({ isDirectory: () => true } as any);
}); });
@@ -55,6 +87,7 @@ describe('terminal-service.ts', () => {
describe('detectShell', () => { describe('detectShell', () => {
it('should detect PowerShell Core on Windows when available', () => { it('should detect PowerShell Core on Windows when available', () => {
vi.mocked(os.platform).mockReturnValue('win32'); vi.mocked(os.platform).mockReturnValue('win32');
vi.mocked(platform.getShellPaths).mockReturnValue(windowsShellPaths);
vi.mocked(platform.systemPathExists).mockImplementation((path: string) => { vi.mocked(platform.systemPathExists).mockImplementation((path: string) => {
return path === 'C:\\Program Files\\PowerShell\\7\\pwsh.exe'; 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', () => { it('should fall back to PowerShell on Windows if Core not available', () => {
vi.mocked(os.platform).mockReturnValue('win32'); vi.mocked(os.platform).mockReturnValue('win32');
vi.mocked(platform.getShellPaths).mockReturnValue(windowsShellPaths);
vi.mocked(platform.systemPathExists).mockImplementation((path: string) => { vi.mocked(platform.systemPathExists).mockImplementation((path: string) => {
return path === 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe'; 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', () => { it('should fall back to cmd.exe on Windows if no PowerShell', () => {
vi.mocked(os.platform).mockReturnValue('win32'); vi.mocked(os.platform).mockReturnValue('win32');
vi.mocked(platform.getShellPaths).mockReturnValue(windowsShellPaths);
vi.mocked(platform.systemPathExists).mockReturnValue(false); vi.mocked(platform.systemPathExists).mockReturnValue(false);
const result = service.detectShell(); const result = service.detectShell();
@@ -114,7 +149,10 @@ describe('terminal-service.ts', () => {
it('should fall back to bash on macOS if zsh not available', () => { it('should fall back to bash on macOS if zsh not available', () => {
vi.mocked(os.platform).mockReturnValue('darwin'); vi.mocked(os.platform).mockReturnValue('darwin');
vi.spyOn(process, 'env', 'get').mockReturnValue({}); 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(); const result = service.detectShell();

View File

@@ -109,21 +109,45 @@ export function getClaudeProjectsDir(): string {
/** /**
* Get common shell paths for shell detection * Get common shell paths for shell detection
* Includes both full paths and short names to match $SHELL or PATH entries
*/ */
export function getShellPaths(): string[] { export function getShellPaths(): string[] {
if (process.platform === 'win32') { if (process.platform === 'win32') {
return [ return [
process.env.COMSPEC || 'cmd.exe', // Full paths (most specific first)
'cmd.exe',
'powershell.exe',
'pwsh.exe', // PowerShell Core short form
'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe',
'C:\\Program Files\\PowerShell\\7\\pwsh.exe', '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',
];
} }
// ============================================================================= // =============================================================================