feat: configurable terminal shell with Windows pwsh auto-probe (#95)
* feat: configurable terminal shell with Windows pwsh auto-probe Windows previously hardcoded powershell.exe (5.1) in defaultShell(), ignoring PowerShell 7 installs. Resolve the shell in priority order: explicit shell config -> DSH_SIDEBAR_SHELL -> PATH/known-dir probe for pwsh.exe -> inbox 5.1 fallback. The same resolved shell feeds both the UI terminal tabs (PtyManager) and the model-facing terminal_* tools (AgentPtyRegistry). defaultShell() now accepts injectable platform/env/exists options so the Windows chain is unit-testable on the ubuntu CI, which never executes win32 branches. The probe also checks ProgramW6432 for 32-bit Node, and machines without PowerShell 7 keep the 5.1 fallback. Refs: #46 * fix: trim resolved SHELL and document ProgramW6432 in resolver contract - POSIX SHELL is validated with trim() but was returned untrimmed; trailing whitespace could leak into the spawned executable path. Return the trimmed value like the explicit/config and Windows/env branches. - ShellResolutionOptions.env comment now lists ProgramW6432, which the Windows probe prefers over ProgramFiles on 32-bit Node. - Test pins the SHELL trimming behavior.
This commit is contained in:
@@ -48,6 +48,14 @@ export interface SidebarConfig {
|
||||
terminalsPerSession?: number
|
||||
/** How long a disconnected terminal process survives awaiting a reconnect. */
|
||||
reconnectGraceMs?: number
|
||||
/**
|
||||
* Terminal shell (absolute path or bare executable name) for BOTH the UI
|
||||
* terminal tabs and the model-facing `terminal_*` tools. Empty = auto:
|
||||
* POSIX follows `$SHELL` then the account login shell; Windows follows
|
||||
* `DSH_SIDEBAR_SHELL`, then probes for `pwsh.exe`, then falls back to the
|
||||
* inbox `powershell.exe` (5.1).
|
||||
*/
|
||||
shell?: string
|
||||
}
|
||||
|
||||
/** Schemastery schema for the plugin configuration. */
|
||||
@@ -57,6 +65,7 @@ export const Config: z<SidebarConfig> = z.object({
|
||||
listLimit: z.number().step(1).min(1).default(1000),
|
||||
terminalsPerSession: z.number().step(1).min(1).default(3),
|
||||
reconnectGraceMs: z.number().step(1).min(0).default(30_000),
|
||||
shell: z.string().default(''),
|
||||
})
|
||||
|
||||
/** Fully defaulted sidebar host settings. */
|
||||
@@ -66,6 +75,7 @@ export interface ResolvedSidebarConfig {
|
||||
listLimit: number
|
||||
terminalsPerSession: number
|
||||
reconnectGraceMs: number
|
||||
shell: string
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -81,6 +91,7 @@ export function resolveSidebarConfig(config: SidebarConfig | undefined): Resolve
|
||||
listLimit: config?.listLimit ?? 1000,
|
||||
terminalsPerSession: config?.terminalsPerSession ?? 3,
|
||||
reconnectGraceMs: config?.reconnectGraceMs ?? 30_000,
|
||||
shell: config?.shell?.trim() ?? '',
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+7
-2
@@ -426,18 +426,23 @@ export function apply(ctx: Context, config?: SidebarConfig): void {
|
||||
// restore it before any terminal can spawn (idempotent).
|
||||
ensureSpawnHelper()
|
||||
const resolved = resolveSidebarConfig(config)
|
||||
// One shell resolution feeds BOTH terminal surfaces: the UI tabs and the
|
||||
// model-facing terminal_* tools. They must stay in lockstep, otherwise a
|
||||
// configured shell fixes one surface and silently leaves the other on the
|
||||
// platform default.
|
||||
const terminalShell = defaultShell({ explicit: resolved.shell })
|
||||
// The web runtime's bind-derived trust list (boot-sampled LAN literals
|
||||
// plus --trusted-host authorities) — the authoritative source the /api
|
||||
// gateway fence derives its list from. Read per request from the live
|
||||
// service value; a replaced list takes effect without a plugin restart.
|
||||
const fence = (req: SidebarHttpRequest): boolean => isTrustedApiRequest(req, ctx.webRuntime.trustedHosts)
|
||||
const ptyManager = new PtyManager(defaultShell(), resolved.terminalsPerSession)
|
||||
const ptyManager = new PtyManager(terminalShell, resolved.terminalsPerSession)
|
||||
// The agent-owned terminal registry: parallel to the UI-tab ptyManager,
|
||||
// keyed by uuid (the model's opaque handle) instead of `${sessionId}:${tabId}`,
|
||||
// uncapped, and torn down with the plugin. The model creates terminals here
|
||||
// through the terminal_create tool; the sidebar view attaches through the
|
||||
// same /sidebar/ws/terminal upgrade with ?uuid=... instead of ?tab=...
|
||||
const agentPtyRegistry = new AgentPtyRegistry(defaultShell())
|
||||
const agentPtyRegistry = new AgentPtyRegistry(terminalShell)
|
||||
|
||||
// ── User-facing "Side card" preferences ──────────────────────────────────
|
||||
// Register the namespace with the settings provider so the Settings page
|
||||
|
||||
+86
-11
@@ -193,18 +193,93 @@ export class PtyManager {
|
||||
}
|
||||
|
||||
/**
|
||||
* The interactive shell for this platform, resolved like a terminal
|
||||
* emulator: an explicit `$SHELL` on the dsh process wins (deployment
|
||||
* override), then the account's login shell from passwd, then `/bin/bash`.
|
||||
* The passwd step matters because service managers and container inits
|
||||
* often start dsh without `SHELL`, and the tab should still open the
|
||||
* user's login shell (e.g. zsh) instead of silently degrading to bash.
|
||||
* Windows short-circuits to `powershell.exe` before any resolution.
|
||||
* Inputs for {@link defaultShell} resolution. Every field is optional and
|
||||
* defaults to the live process, which keeps the no-argument call sites
|
||||
* working while tests (and exotic embedders) can pin the platform, the
|
||||
* environment, and the existence probe independently — the Windows chain
|
||||
* never executes on the ubuntu CI runners, so it is only testable through
|
||||
* these injection points.
|
||||
*/
|
||||
export function defaultShell(): string {
|
||||
if (process.platform === 'win32') return 'powershell.exe'
|
||||
const envShell = process.env.SHELL
|
||||
if (envShell !== undefined && envShell.trim() !== '') return envShell
|
||||
export interface ShellResolutionOptions {
|
||||
/** Platform override (defaults to `process.platform`). */
|
||||
platform?: NodeJS.Platform
|
||||
/** Environment override; the resolver only reads SHELL, DSH_SIDEBAR_SHELL, PATH, ProgramW6432, ProgramFiles, LOCALAPPDATA. */
|
||||
env?: NodeJS.ProcessEnv
|
||||
/** Explicitly configured shell (the `shell` config field); wins over every automatic source. Empty means unset. */
|
||||
explicit?: string
|
||||
/** File-existence probe override (defaults to `existsSync`). */
|
||||
exists?: (path: string) => boolean
|
||||
}
|
||||
|
||||
/**
|
||||
* Candidate directories that may contain a `pwsh.exe` on Windows: PATH
|
||||
* entries first, then the well-known machine/user install locations
|
||||
* (including preview channels and per-user MSI/portable layouts). The
|
||||
* machine-scope search reads both `ProgramW6432` and `ProgramFiles` so a
|
||||
* 32-bit Node process — whose `ProgramFiles` points at `(x86)` — still
|
||||
* finds a 64-bit PowerShell 7 install. De-duped while preserving priority
|
||||
* order.
|
||||
*/
|
||||
function windowsPwshCandidateDirs(env: NodeJS.ProcessEnv): string[] {
|
||||
const dirs: string[] = []
|
||||
const pathEntries = 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
|
||||
// injection point.
|
||||
for (const entry of pathEntries.split(';')) {
|
||||
const trimmed = entry.trim()
|
||||
if (trimmed !== '') dirs.push(trimmed)
|
||||
}
|
||||
}
|
||||
for (const programFiles of [env.ProgramW6432, 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
|
||||
if (localAppData !== undefined && localAppData.trim() !== '') {
|
||||
dirs.push(join(localAppData, 'Microsoft', 'PowerShell', '7'))
|
||||
dirs.push(join(localAppData, 'Microsoft', 'PowerShell', '7-preview'))
|
||||
dirs.push(join(localAppData, 'Programs', 'PowerShell', '7'))
|
||||
dirs.push(join(localAppData, 'Programs', 'PowerShell', '7-preview'))
|
||||
}
|
||||
return [...new Set(dirs)]
|
||||
}
|
||||
|
||||
/**
|
||||
* The interactive shell for this platform, resolved like a terminal
|
||||
* emulator: an explicitly configured shell (the `shell` config field) wins,
|
||||
* then `$SHELL` on POSIX (deployment override), then the account's login
|
||||
* shell from passwd, then `/bin/bash`. The passwd step matters because
|
||||
* service managers and container inits often start dsh without `SHELL`, and
|
||||
* the tab should still open the user's login shell (e.g. zsh) instead of
|
||||
* silently degrading to bash.
|
||||
*
|
||||
* Windows previously short-circuited to `powershell.exe` (the inbox 5.1)
|
||||
* before any resolution, so PowerShell 7 users always got a legacy shell
|
||||
* without `??`/`?.`/ternary and with poor ANSI/UTF-8 defaults. The Windows
|
||||
* chain is now: explicit shell → `DSH_SIDEBAR_SHELL` env override → first
|
||||
* `pwsh.exe` found on PATH or in a known install directory → the 5.1
|
||||
* fallback (machines without PowerShell 7 keep working).
|
||||
*/
|
||||
export function defaultShell(options: ShellResolutionOptions = {}): string {
|
||||
const platform = options.platform ?? process.platform
|
||||
const env = options.env ?? process.env
|
||||
const exists = options.exists ?? existsSync
|
||||
const explicit = options.explicit
|
||||
if (explicit !== undefined && explicit.trim() !== '') return explicit.trim()
|
||||
if (platform === 'win32') {
|
||||
const envShell = env.DSH_SIDEBAR_SHELL
|
||||
if (envShell !== undefined && envShell.trim() !== '') return envShell.trim()
|
||||
for (const dir of windowsPwshCandidateDirs(env)) {
|
||||
const candidate = join(dir, 'pwsh.exe')
|
||||
if (exists(candidate)) return candidate
|
||||
}
|
||||
return 'powershell.exe'
|
||||
}
|
||||
const envShell = env.SHELL
|
||||
if (envShell !== undefined && envShell.trim() !== '') return envShell.trim()
|
||||
// userInfo() throws when the uid has no passwd entry (rare chroots);
|
||||
// without a login shell there is nothing better than the bash default.
|
||||
try {
|
||||
|
||||
@@ -34,6 +34,13 @@ describe('dsh-better-sidebar plugin export shape', () => {
|
||||
expect(resolved.listLimit).toBe(1000)
|
||||
expect(resolved.terminalsPerSession).toBe(3)
|
||||
expect(resolved.reconnectGraceMs).toBe(30_000)
|
||||
// The terminal shell config defaults to auto-resolution (empty shell =
|
||||
// the platform chain in defaultShell()).
|
||||
expect(resolved.shell).toBe('')
|
||||
const configured = (schema as unknown as {
|
||||
(input: Record<string, unknown> | undefined): Record<string, unknown>
|
||||
})({ shell: 'pwsh.exe' })
|
||||
expect(configured.shell).toBe('pwsh.exe')
|
||||
})
|
||||
|
||||
it('registers the side card preferences schema with the documented defaults', async () => {
|
||||
|
||||
+54
-19
@@ -31,6 +31,7 @@ import { wrapOpenPath, type OpenPathInterceptDeps, type OpenPathService } from '
|
||||
import { registerOpenPathInterception } from '../src/client/intercept.tsx'
|
||||
import type { Context } from '../src/context-types.ts'
|
||||
import { defaultShell, ensureSpawnHelper } from '../src/pty-manager.ts'
|
||||
import { resolveSidebarConfig } from '../src/config.ts'
|
||||
import {
|
||||
collectBranchIds, countSubagentDescendants, detectNewDirectSubagent, directSubagentCount, rootAncestor,
|
||||
} from '../src/client/subagent-detect.ts'
|
||||
@@ -859,25 +860,59 @@ describe('agent terminal reconciliation', () => {
|
||||
})
|
||||
|
||||
describe('pty helpers', () => {
|
||||
it('prefers an explicit SHELL, then falls back to the account login shell', () => {
|
||||
if (process.platform === 'win32') {
|
||||
// defaultShell() short-circuits to powershell.exe before env/passwd
|
||||
// resolution on Windows; the POSIX fallback chain is asserted below.
|
||||
expect(defaultShell()).toBe('powershell.exe')
|
||||
return
|
||||
}
|
||||
const previous = process.env.SHELL
|
||||
try {
|
||||
process.env.SHELL = '/explicit/zsh'
|
||||
expect(defaultShell()).toBe('/explicit/zsh')
|
||||
process.env.SHELL = ' '
|
||||
expect(defaultShell()).toBe('/usr/bin/zsh')
|
||||
delete process.env.SHELL
|
||||
expect(defaultShell()).toBe('/usr/bin/zsh')
|
||||
} finally {
|
||||
if (previous === undefined) delete process.env.SHELL
|
||||
else process.env.SHELL = previous
|
||||
}
|
||||
it('prefers an explicit shell, then SHELL, then the account login shell on POSIX', () => {
|
||||
// The platform is injected so this chain also runs (and is asserted) on
|
||||
// non-POSIX developer machines. The userInfo mock at the top of the file
|
||||
// pins the passwd login shell to /usr/bin/zsh.
|
||||
expect(defaultShell({ platform: 'linux', env: { SHELL: '/explicit/zsh' } })).toBe('/explicit/zsh')
|
||||
// Surrounding whitespace must not leak into the spawned executable path.
|
||||
expect(defaultShell({ platform: 'linux', env: { SHELL: ' /explicit/zsh ' } })).toBe('/explicit/zsh')
|
||||
expect(defaultShell({ platform: 'linux', env: { SHELL: ' ' } })).toBe('/usr/bin/zsh')
|
||||
expect(defaultShell({ platform: 'linux', env: {} })).toBe('/usr/bin/zsh')
|
||||
})
|
||||
|
||||
it('Windows: explicit shell wins, then DSH_SIDEBAR_SHELL, then a probed pwsh.exe, then powershell.exe 5.1', () => {
|
||||
// The explicit shell (the `shell` config field) beats every automatic
|
||||
// source, and whitespace-only values count as unset.
|
||||
expect(defaultShell({ platform: 'win32', explicit: 'C:\\Tools\\pwsh.exe', env: { DSH_SIDEBAR_SHELL: 'pwsh.exe' } }))
|
||||
.toBe('C:\\Tools\\pwsh.exe')
|
||||
expect(defaultShell({ platform: 'win32', explicit: ' ', env: { DSH_SIDEBAR_SHELL: 'pwsh.exe' } }))
|
||||
.toBe('pwsh.exe')
|
||||
|
||||
// PATH is probed entry by entry; the first directory containing pwsh.exe
|
||||
// wins and the returned value is the full resolved path. The candidate is
|
||||
// normalized to forward slashes before comparing so the assertion holds
|
||||
// under BOTH join() implementations: on the ubuntu runners node:path is
|
||||
// POSIX and joins 'C:\\other' + 'pwsh.exe' as 'C:\\other/pwsh.exe'.
|
||||
const fromPath = defaultShell({
|
||||
platform: 'win32',
|
||||
env: { PATH: 'C:\\tools;C:\\other' },
|
||||
exists: path => path.replaceAll('\\', '/') === 'C:/other/pwsh.exe',
|
||||
})
|
||||
expect(fromPath.replaceAll('\\', '/')).toBe('C:/other/pwsh.exe')
|
||||
|
||||
// PATH misses fall through to the known install directories. On 32-bit
|
||||
// Node, ProgramW6432 is the real 64-bit Program Files and is preferred.
|
||||
const fromProgramW6432 = defaultShell({
|
||||
platform: 'win32',
|
||||
env: { ProgramW6432: 'C:\\PF64' },
|
||||
exists: path => path.replaceAll('\\', '/') === 'C:/PF64/PowerShell/7/pwsh.exe',
|
||||
})
|
||||
expect(fromProgramW6432.replaceAll('\\', '/')).toBe('C:/PF64/PowerShell/7/pwsh.exe')
|
||||
const fromProgramFiles = defaultShell({
|
||||
platform: 'win32',
|
||||
env: { ProgramFiles: 'C:\\PF', LOCALAPPDATA: 'C:\\Users\\x\\AppData\\Local' },
|
||||
exists: path => path.replaceAll('\\', '/') === 'C:/PF/PowerShell/7/pwsh.exe',
|
||||
})
|
||||
expect(fromProgramFiles.replaceAll('\\', '/')).toBe('C:/PF/PowerShell/7/pwsh.exe')
|
||||
|
||||
// Nothing installed: keep the inbox 5.1 fallback instead of breaking.
|
||||
expect(defaultShell({ platform: 'win32', env: {}, exists: () => false })).toBe('powershell.exe')
|
||||
})
|
||||
|
||||
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')
|
||||
})
|
||||
|
||||
it('restores the spawn-helper executable bit idempotently', () => {
|
||||
|
||||
Reference in New Issue
Block a user