From 74993839e7cd5d0449429d42012d2c3a70d33adf Mon Sep 17 00:00:00 2001 From: jackie-cqz <2557911191@qq.com> Date: Wed, 2 Sep 2026 00:42:43 +0800 Subject: [PATCH] fix(terminal): resolve Windows custom shell executables (#487) --- src/agent-pty.ts | 5 +- src/pty-manager.ts | 103 ++++++++++++++++++++++++++++++++++++-- tests/pty-helpers.spec.ts | 49 +++++++++++++++++- 3 files changed, 149 insertions(+), 8 deletions(-) diff --git a/src/agent-pty.ts b/src/agent-pty.ts index c5d8393..e7a0e72 100644 --- a/src/agent-pty.ts +++ b/src/agent-pty.ts @@ -13,7 +13,7 @@ */ import { randomUUID } from 'node:crypto' import type { IPty } from 'node-pty' -import { ensureSpawnHelper, shellSpawnArgs } from './pty-manager.ts' +import { ensureSpawnHelper, resolveShellExecutable, shellSpawnArgs } from './pty-manager.ts' import { loadRequiredNodePty, type NodePtyModule } from './pty-deps.ts' import { SidebarError } from './wire.ts' @@ -241,7 +241,8 @@ export class AgentPtyRegistry { ): string { const uuid = randomUUID() const dims = clampDims(cols, rows) - const pty = this.nodePty.spawn(shell ?? this.shell, shellSpawnArgs(shellArgs ?? this.shellArgs), { + const executable = resolveShellExecutable(shell ?? this.shell) + const pty = this.nodePty.spawn(executable, shellSpawnArgs(shellArgs ?? this.shellArgs), { name: 'xterm-256color', cols: dims.cols, rows: dims.rows, diff --git a/src/pty-manager.ts b/src/pty-manager.ts index 0b0a068..5d5712d 100644 --- a/src/pty-manager.ts +++ b/src/pty-manager.ts @@ -7,7 +7,7 @@ * tab is closed or the plugin tears down. */ import { chmodSync, existsSync } from 'node:fs' -import { dirname, join } from 'node:path' +import { dirname, join, win32 as win32Path } from 'node:path' import { createRequire } from 'node:module' import { userInfo } from 'node:os' import type { IPty } from 'node-pty' @@ -148,12 +148,13 @@ export class PtyManager { if (this.keysOf(sessionId).length >= this.maxPerSession) { throw new SidebarError('pty-error', `terminal limit reached (${this.maxPerSession}) for this session`, 400) } + const executable = resolveShellExecutable(shell ?? this.shell) const handle: SidebarPty = { key, sessionId, tabId, cwd, - pty: this.nodePty.spawn(shell ?? this.shell, shellSpawnArgs(shellArgs ?? this.shellArgs), { + pty: this.nodePty.spawn(executable, shellSpawnArgs(shellArgs ?? this.shellArgs), { name: 'xterm-256color', cols: Math.max(2, Math.floor(cols)), rows: Math.max(2, Math.floor(rows)), @@ -270,6 +271,32 @@ export interface ShellResolutionOptions { exists?: (path: string) => boolean } +/** Inputs for resolving one configured shell into the executable path passed + * to node-pty. Injectable so the Windows-only search semantics stay covered + * on POSIX CI runners. */ +export interface ShellExecutableResolutionOptions { + /** Platform override (defaults to `process.platform`). */ + platform?: NodeJS.Platform + /** Environment override; Windows reads PATH/PATHEXT/SystemRoot plus the + * PowerShell well-known-location variables. */ + env?: NodeJS.ProcessEnv + /** File-existence probe override (defaults to `existsSync`). */ + exists?: (path: string) => boolean +} + +/** Read one Windows environment value case-insensitively. Real + * `process.env` has case-insensitive lookup on Windows, but injected objects + * and some embedders do not preserve that behavior. */ +function windowsEnv(env: NodeJS.ProcessEnv, name: string): string | undefined { + const direct = env[name] + if (direct !== undefined) return direct + const lowered = name.toLowerCase() + for (const [key, value] of Object.entries(env)) { + if (key.toLowerCase() === lowered) return value + } + return undefined +} + /** * Candidate directories that may contain a `pwsh.exe` on Windows: PATH * entries first, then the well-known machine/user install locations @@ -281,7 +308,7 @@ export interface ShellResolutionOptions { */ function windowsPwshCandidateDirs(env: NodeJS.ProcessEnv): string[] { const dirs: string[] = [] - const pathEntries = env.PATH + const pathEntries = windowsEnv(env, 'PATH') if (pathEntries !== undefined) { // The win32 branch always uses the Windows PATH separator; hardcoding it // keeps the function testable from POSIX runners without a delimiter @@ -291,12 +318,12 @@ function windowsPwshCandidateDirs(env: NodeJS.ProcessEnv): string[] { if (trimmed !== '') dirs.push(trimmed) } } - for (const programFiles of [env.ProgramW6432, env.ProgramFiles]) { + for (const programFiles of [windowsEnv(env, 'ProgramW6432'), windowsEnv(env, 'ProgramFiles')]) { if (programFiles === undefined || programFiles.trim() === '') continue dirs.push(join(programFiles, 'PowerShell', '7')) dirs.push(join(programFiles, 'PowerShell', '7-preview')) } - const localAppData = env.LOCALAPPDATA + const localAppData = windowsEnv(env, 'LOCALAPPDATA') if (localAppData !== undefined && localAppData.trim() !== '') { dirs.push(join(localAppData, 'Microsoft', 'PowerShell', '7')) dirs.push(join(localAppData, 'Microsoft', 'PowerShell', '7-preview')) @@ -306,6 +333,72 @@ function windowsPwshCandidateDirs(env: NodeJS.ProcessEnv): string[] { return [...new Set(dirs)] } +/** + * Resolve the configured shell executable before handing it to node-pty. + * + * POSIX node-pty uses `execvp`, so bare commands already follow PATH and are + * passed through unchanged. Windows' native backend does not consistently + * apply the shell's PATHEXT lookup to a bare value (`pwsh` / `cmd` can fail + * with the opaque `File not found:` error), so perform the lookup ourselves: + * + * - an explicit path is accepted as-is when it exists (or with a PATHEXT + * suffix when the user omitted `.exe`), + * - a bare name is searched through PATH, System32, and PowerShell's known + * install directories, + * - failure becomes a stable, actionable pty-error instead of a native + * backend string with no mention of the configured shell. + */ +export function resolveShellExecutable( + shell: string, + options: ShellExecutableResolutionOptions = {}, +): string { + const configured = shell.trim() + const platform = options.platform ?? process.platform + if (platform !== 'win32' || configured === '') return configured + + const env = options.env ?? process.env + const exists = options.exists ?? existsSync + const rawPathext = windowsEnv(env, 'PATHEXT') + const executableExts = (rawPathext ?? '.COM;.EXE') + .split(';') + .map(extension => extension.trim()) + // node-pty ultimately calls CreateProcess; batch files need an + // intermediate cmd.exe and therefore are not valid shell executables. + .filter(extension => /^\.(?:com|exe)$/i.test(extension)) + if (executableExts.length === 0) executableExts.push('.EXE', '.COM') + + const hasExtension = win32Path.extname(configured) !== '' + const names = hasExtension + ? [configured] + : executableExts.map(extension => configured + extension.toLowerCase()) + const hasPath = win32Path.isAbsolute(configured) || /[\\/]/.test(configured) + const candidates: string[] = [] + if (hasPath) { + candidates.push(...names) + } else { + const path = windowsEnv(env, 'PATH') + if (path !== undefined) { + for (const dir of path.split(';').map(entry => entry.trim()).filter(Boolean)) { + for (const name of names) candidates.push(win32Path.join(dir, name)) + } + } + const systemRoot = windowsEnv(env, 'SystemRoot') + if (systemRoot !== undefined && systemRoot.trim() !== '') { + for (const name of names) candidates.push(win32Path.join(systemRoot, 'System32', name)) + } + if (/^pwsh(?:\.exe)?$/i.test(configured)) { + for (const dir of windowsPwshCandidateDirs(env)) { + candidates.push(win32Path.join(dir, 'pwsh.exe')) + } + } + } + + for (const candidate of [...new Set(candidates)]) { + if (exists(candidate)) return candidate + } + throw new SidebarError('pty-error', `shell executable not found: "${configured}"`) +} + /** * The interactive shell for this platform, resolved like a terminal * emulator: an explicitly configured shell (the `shell` config field) wins, diff --git a/tests/pty-helpers.spec.ts b/tests/pty-helpers.spec.ts index d0d8186..e1f4267 100644 --- a/tests/pty-helpers.spec.ts +++ b/tests/pty-helpers.spec.ts @@ -13,7 +13,13 @@ vi.mock('node:os', async (importOriginal) => { }) import { resolveSidebarConfig } from '../src/config.ts' -import { defaultShell, ensureSpawnHelper, shellDisplayName, shellSpawnArgs } from '../src/pty-manager.ts' +import { + defaultShell, + ensureSpawnHelper, + resolveShellExecutable, + shellDisplayName, + shellSpawnArgs, +} from '../src/pty-manager.ts' describe('pty helpers', () => { it('prefers an explicit shell, then SHELL, then the account login shell on POSIX', () => { @@ -66,6 +72,47 @@ describe('pty helpers', () => { expect(defaultShell({ platform: 'win32', env: {}, exists: () => false })).toBe('powershell.exe') }) + it('Windows: resolves bare custom shells through PATH/PATHEXT before node-pty spawn', () => { + const files = new Set([ + 'C:/Tools/PowerShell/pwsh.exe', + 'C:/Windows/System32/cmd.exe', + ]) + const options = { + platform: 'win32' as const, + // Mixed-case Path/PATHEXT pins the case-insensitive environment lookup + // that real Windows process.env provides but plain test objects do not. + env: { + Path: 'C:\\Tools\\PowerShell', + PathExt: '.COM;.EXE;.BAT;.CMD', + SystemRoot: 'C:\\Windows', + }, + exists: (path: string) => files.has(path.replaceAll('\\', '/')), + } + expect(resolveShellExecutable('pwsh', options).replaceAll('\\', '/')) + .toBe('C:/Tools/PowerShell/pwsh.exe') + expect(resolveShellExecutable('cmd', options).replaceAll('\\', '/')) + .toBe('C:/Windows/System32/cmd.exe') + }) + + it('Windows: accepts .exe names and absolute paths, and names a missing configured shell', () => { + const options = { + platform: 'win32' as const, + env: { PATH: 'C:\\Tools' }, + exists: (path: string) => path.replaceAll('\\', '/') === 'C:/Tools/pwsh.exe' + || path.replaceAll('\\', '/') === 'D:/Portable Shell/custom.exe', + } + expect(resolveShellExecutable('pwsh.exe', options).replaceAll('\\', '/')) + .toBe('C:/Tools/pwsh.exe') + expect(resolveShellExecutable('D:\\Portable Shell\\custom.exe', options).replaceAll('\\', '/')) + .toBe('D:/Portable Shell/custom.exe') + expect(() => resolveShellExecutable('missing-shell', options)) + .toThrow('shell executable not found: "missing-shell"') + }) + + it('keeps POSIX bare shell resolution delegated to execvp', () => { + expect(resolveShellExecutable(' zsh ', { platform: 'linux', env: {}, exists: () => false })).toBe('zsh') + }) + it('trims the configured shell and defaults it to auto for old documents', () => { expect(resolveSidebarConfig(undefined).shell).toBe('') expect(resolveSidebarConfig({ shell: ' pwsh.exe ' }).shell).toBe('pwsh.exe')