From 76fa7df2641f911a67e84100cabdf7e9849f0f09 Mon Sep 17 00:00:00 2001 From: NolanHo Date: Fri, 14 Aug 2026 10:01:30 -0700 Subject: [PATCH] fix: resolve the account login shell and spawn terminals as login shells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Terminal tabs resolved the shell from the dsh process environment only, falling back to a hardcoded /bin/bash — service-managed launches without SHELL therefore ignored the account's login shell (e.g. zsh). They also spawned with an empty argv, so even a resolved zsh ran as a non-login shell and skipped ~/.zprofile. defaultShell() now falls back to the account's login shell (passwd) after $SHELL, and both registries spawn POSIX shells with -l so the tab behaves like a terminal emulator (loads ~/.profile / ~/.zprofile). Windows keeps powershell.exe with no login flag. Fixes omdsh-dev/DSH-better-sidebar#68 --- src/agent-pty.ts | 4 ++-- src/pty-manager.ts | 35 +++++++++++++++++++++++++++++++---- tests/smoke.spec.ts | 28 ++++++++++++++++++++++++++++ tests/unit.spec.ts | 31 ++++++++++++++++++++++++++----- 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/src/agent-pty.ts b/src/agent-pty.ts index 5f94231..23cc28f 100644 --- a/src/agent-pty.ts +++ b/src/agent-pty.ts @@ -13,7 +13,7 @@ */ import { randomUUID } from 'node:crypto' import * as nodePty from 'node-pty' -import { ensureSpawnHelper } from './pty-manager.ts' +import { ensureSpawnHelper, shellSpawnArgs } from './pty-manager.ts' import { SidebarError } from './wire.ts' /** Per-agent-terminal transcript bound (bytes kept for replay and reads). */ @@ -211,7 +211,7 @@ export class AgentPtyRegistry { ): string { const uuid = randomUUID() const dims = clampDims(cols, rows) - const pty = nodePty.spawn(this.shell, [], { + const pty = nodePty.spawn(this.shell, shellSpawnArgs(), { name: 'xterm-256color', cols: dims.cols, rows: dims.rows, diff --git a/src/pty-manager.ts b/src/pty-manager.ts index bd483e6..5b617ec 100644 --- a/src/pty-manager.ts +++ b/src/pty-manager.ts @@ -9,6 +9,7 @@ import { chmodSync, existsSync } from 'node:fs' import { dirname, join } from 'node:path' import { createRequire } from 'node:module' +import { userInfo } from 'node:os' import * as nodePty from 'node-pty' import { SidebarError } from './wire.ts' @@ -118,7 +119,7 @@ export class PtyManager { sessionId, tabId, cwd, - pty: nodePty.spawn(this.shell, [], { + pty: nodePty.spawn(this.shell, shellSpawnArgs(), { name: 'xterm-256color', cols: Math.max(2, Math.floor(cols)), rows: Math.max(2, Math.floor(rows)), @@ -191,9 +192,35 @@ export class PtyManager { } } -/** The interactive shell for this platform (empty SHELL falls back). */ +/** + * 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. + */ export function defaultShell(): string { if (process.platform === 'win32') return 'powershell.exe' - const shell = process.env.SHELL - return shell !== undefined && shell.trim() !== '' ? shell : '/bin/bash' + const envShell = process.env.SHELL + if (envShell !== undefined && envShell.trim() !== '') return envShell + // userInfo() throws when the uid has no passwd entry (rare chroots); + // without a login shell there is nothing better than the bash default. + try { + const loginShell = userInfo().shell + if (typeof loginShell === 'string' && loginShell.trim() !== '') return loginShell + } catch { + // no passwd entry: fall through to /bin/bash + } + return '/bin/bash' +} + +/** + * Spawn arguments that make the shell behave like a terminal-emulator tab: + * POSIX shells start as login shells (`-l`) so they read the profile files + * (`~/.profile`, `~/.zprofile`); Windows PowerShell takes no login flag. + */ +export function shellSpawnArgs(): string[] { + return process.platform === 'win32' ? [] : ['-l'] } diff --git a/tests/smoke.spec.ts b/tests/smoke.spec.ts index ef13e3a..c6f08d6 100644 --- a/tests/smoke.spec.ts +++ b/tests/smoke.spec.ts @@ -188,6 +188,34 @@ describe('host plugin smoke', () => { } }) + it.skipIf(process.platform === 'win32')('spawns the shell as a login shell (loads ~/.profile)', async () => { + const home = mkdtempSync(join(tmpdir(), 'dsh-sidebar-login-')) + const previousHome = process.env.HOME + try { + // A login bash reads ~/.profile (a non-login interactive bash reads + // ~/.bashrc instead), so this marker proves the spawn used a login + // argv — the terminal-emulator behavior the tab should match. + writeFileSync(join(home, '.profile'), 'export DSH_LOGIN_MARKER=loaded-from-profile\n') + process.env.HOME = home + const manager = new PtyManager('/bin/bash', 3) + try { + const handle = manager.open('s5', 't1', process.cwd(), 80, 24) + handle.pty.write('echo $DSH_LOGIN_MARKER\r') + const deadline = Date.now() + 5000 + while (!handle.transcript.includes('loaded-from-profile') && Date.now() < deadline) { + await new Promise(resolve => setTimeout(resolve, 50)) + } + expect(handle.transcript).toContain('loaded-from-profile') + } finally { + manager.disposeAll() + } + } finally { + if (previousHome === undefined) delete process.env.HOME + else process.env.HOME = previousHome + rmSync(home, { recursive: true, force: true }) + } + }) + it('lists the repository root level', async () => { const listing = await listDirectory(process.cwd(), 1000) expect(listing.entries.some(entry => entry.name === 'src' && entry.isDir)).toBe(true) diff --git a/tests/unit.spec.ts b/tests/unit.spec.ts index 9c88765..d430b4f 100644 --- a/tests/unit.spec.ts +++ b/tests/unit.spec.ts @@ -1,4 +1,17 @@ -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// Pin the passwd login shell to a non-bash value for the pty helpers tests: +// the CI runner's own login shell is /bin/bash, which the old hardcoded +// fallback would have satisfied by coincidence, so the fallback chain needs +// a mock passwd to be genuinely discriminating. No other test in this file +// reads os.userInfo. +vi.mock('node:os', async (importOriginal) => { + const os = await importOriginal() + return { + ...os, + userInfo: () => ({ ...os.userInfo(), shell: '/usr/bin/zsh' }), + } +}) import { compareEntries, isWithin, parentOf, rootLabel, requireAbsolute } from '../src/fs-tree.ts' import { parseLogLines, parsePorcelainZ } from '../src/git.ts' import { parseUnifiedDiff } from '../src/client/DiffView.tsx' @@ -846,13 +859,21 @@ describe('agent terminal reconciliation', () => { }) describe('pty helpers', () => { - it('falls back from an empty SHELL to a usable shell', () => { + 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 = '' - expect(defaultShell()).toBe('/bin/bash') + 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('/bin/bash') + expect(defaultShell()).toBe('/usr/bin/zsh') } finally { if (previous === undefined) delete process.env.SHELL else process.env.SHELL = previous