diff --git a/src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts b/src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts index 16275296dd5..ea94d132409 100644 --- a/src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts +++ b/src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts @@ -9,6 +9,10 @@ import { Session } from './session' const describePosix = process.platform === 'win32' ? describe.skip : describe const hasZsh = process.platform !== 'win32' && spawnSync('/bin/zsh', ['--version']).status === 0 const hasBash = process.platform !== 'win32' && spawnSync('/bin/bash', ['--version']).status === 0 +// script(1) relays to a shell on an inner pty, like kiro-cli-term (#25586). +const hasScript = + process.platform !== 'win32' && + (existsSync('/usr/bin/script') || spawnSync('script', ['--version']).status === 0) const COMMAND_OUTPUT = 'ORCA_STARTUP_COMMAND_RAN' // Shell identity compares realpaths, so a symlink cannot represent a distinct Bash installation. const alternateBashPath = ['/opt/homebrew/bin/bash', '/usr/local/bin/bash', '/usr/bin/bash'].find( @@ -131,10 +135,11 @@ type RunningFixture = { } async function startFixture( - fixture: ShellFixture, + fixture: Pick, startupContent: string, extraFiles: Record = {}, - pathEnv: string = process.env.PATH ?? '/usr/bin:/bin' + pathEnv: string = process.env.PATH ?? '/usr/bin:/bin', + onReadinessEvent?: (event: string) => void ): Promise { const tempHome = mkdtempSync(join(tmpdir(), 'orca-shell-ready-exec-')) const previousHome = process.env.HOME @@ -173,7 +178,8 @@ async function startFixture( cols: 80, rows: 24, subprocess, - shellReadySupported: true + shellReadySupported: true, + ...(onReadinessEvent ? { reportReadinessEvent: onReadinessEvent } : {}) }) let output = '' let onOutput = (): void => {} @@ -437,6 +443,45 @@ fi fi ` + const relayTest = hasScript && hasZsh ? it : it.skip + relayTest( + 'releases the queued command behind a pty relay the exec left in front of the shell', + async () => { + const readinessEvents: string[] = [] + // Drop the wrapper env so the inner shell cannot emit the marker, as with kiro-cli-term. + const scriptRelay = + process.platform === 'darwin' + ? 'script -q /dev/null /bin/zsh -o noglobalrcs -l -i' + : `script -q -c '/bin/zsh -o noglobalrcs -l -i' /dev/null` + const running = await startFixture( + { + name: 'zsh relay', + shellPath: '/bin/zsh', + startupFile: '.zprofile', + command: zshFixture.command + }, + `if [[ -z "\${ORCA_EXEC_REPRO_DONE:-}" ]]; then + export ORCA_EXEC_REPRO_DONE=1 + exec env -u ZDOTDIR -u ORCA_SHELL_FEATURES ${scriptRelay} +fi +`, + {}, + process.env.PATH ?? '/usr/bin:/bin', + (event) => readinessEvents.push(event) + ) + try { + await waitForOutput(running.subscribe, () => running.output().includes(COMMAND_OUTPUT)) + expect(running.session.shellState).toBe('ready') + expect(count(running.output(), COMMAND_OUTPUT)).toBe(1) + expect(readinessEvents).toContain('shell-ready-wrapper-replaced') + expect(readinessEvents).not.toContain('shell-ready-timeout') + } finally { + await running.cleanup() + } + }, + 10_000 + ) + alternateBashTest( 'releases at the prompt of a second Bash install the pane PATH resolves', async () => { diff --git a/src/main/shell-prompt-readiness-probe.test.ts b/src/main/shell-prompt-readiness-probe.test.ts index 56d4b35a06c..908d7f2843d 100644 --- a/src/main/shell-prompt-readiness-probe.test.ts +++ b/src/main/shell-prompt-readiness-probe.test.ts @@ -1,16 +1,20 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const lineEditorProbe = vi.hoisted(() => vi.fn()) +const relayedLineEditorProbe = vi.hoisted(() => vi.fn()) +const listChildPids = vi.hoisted(() => vi.fn()) const processReadinessProbe = vi.hoisted(() => vi.fn()) const resolveExecutablePath = vi.hoisted(() => vi.fn((value: string) => Promise.resolve(value))) const resolveInstalledExecutablePaths = vi.hoisted(() => vi.fn((): Promise => Promise.resolve([])) ) vi.mock('../shared/pty-slave-line-discipline-echo', () => ({ - createPtySlaveLineEditorProbe: () => lineEditorProbe + createPtySlaveLineEditorProbe: (ttyPath: string) => + ttyPath === '/dev/ttys099' ? relayedLineEditorProbe : lineEditorProbe })) vi.mock('../shared/shell-process-readiness', () => ({ readShellProcessReadiness: processReadinessProbe, + listChildProcessIds: listChildPids, resolveShellExecutablePath: resolveExecutablePath, resolveInstalledShellExecutablePaths: resolveInstalledExecutablePaths })) @@ -21,6 +25,8 @@ describe('shell prompt readiness probe', () => { beforeEach(() => { vi.useFakeTimers() lineEditorProbe.mockReset() + relayedLineEditorProbe.mockReset() + listChildPids.mockReset().mockResolvedValue([]) processReadinessProbe.mockReset() resolveExecutablePath.mockClear() resolveInstalledExecutablePaths.mockClear() @@ -97,10 +103,42 @@ describe('shell prompt readiness probe', () => { expect(onPromptReady).not.toHaveBeenCalled() if (terminalState === 'other') { - expect(processReadinessProbe).not.toHaveBeenCalled() + expect(listChildPids).not.toHaveBeenCalled() } }) + it.each([ + ['accepts', 'line-editor', true], + ['rejects', 'other', false] + ])( + '%s a relayed shell whose inner pty is in %s mode (#25586)', + async (_verb, innerState, expected) => { + lineEditorProbe.mockResolvedValue('other') + relayedLineEditorProbe.mockResolvedValue(innerState) + listChildPids.mockResolvedValue([43]) + processReadinessProbe.mockImplementation((pid: number) => + Promise.resolve( + pid === 42 + ? { executablePath: '/usr/local/bin/kiro-cli-term', foreground: true, ttyPath: null } + : { executablePath: '/bin/zsh', foreground: true, ttyPath: '/dev/ttys099' } + ) + ) + const onPromptReady = vi.fn() + const probe = createShellPromptReadinessProbe({ + slavePath: '/dev/ttys048', + shellPath: '/bin/zsh', + getShellPid: () => 42, + onPromptReady, + settleMs: 10 + }) + + probe?.notifyOutput('\x1b[?2004h') + await vi.advanceTimersByTimeAsync(10) + + expect(onPromptReady).toHaveBeenCalledTimes(expected ? 1 : 0) + } + ) + it('accepts a second installation of the same shell that the pane PATH resolves', async () => { lineEditorProbe.mockResolvedValue('line-editor') processReadinessProbe.mockResolvedValue({ diff --git a/src/main/shell-prompt-readiness-probe.ts b/src/main/shell-prompt-readiness-probe.ts index 982fadda4b3..f8c1df43e68 100644 --- a/src/main/shell-prompt-readiness-probe.ts +++ b/src/main/shell-prompt-readiness-probe.ts @@ -1,5 +1,6 @@ import { createPtySlaveLineEditorProbe } from '../shared/pty-slave-line-discipline-echo' import { + listChildProcessIds, readShellProcessReadiness, resolveInstalledShellExecutablePaths, resolveShellExecutablePath @@ -40,49 +41,78 @@ export function createShellPromptReadinessProbe(options: { let generation = 0 let probesStarted = 0 + const isStale = (scheduledGeneration: number): boolean => + disposed || scheduledGeneration !== generation + + const isExpectedShell = async (executablePath: string): Promise => { + if (!expectedShellName || basename(executablePath).toLowerCase() !== expectedShellName) { + return false + } + const expectedPath = options.shellPath + ? await resolveShellExecutablePath(options.shellPath, shellCwd, options.shellPathEnv) + : null + if (!expectedPath) { + return false + } + if (executablePath === expectedPath) { + return true + } + // Profiles can exec another installed shell; accept only replacements found on this pane's PATH. + const installedPaths = await resolveInstalledShellExecutablePaths( + expectedShellName, + shellCwd, + options.shellPathEnv + ) + return installedPaths.includes(executablePath) + } + + // A pty relay exec'd over the shell (kiro-cli-term, #25586) runs it as a child on an inner pty. + const isRelayedShellAtPrompt = async ( + relayPid: number, + scheduledGeneration: number + ): Promise => { + for (const childPid of await listChildProcessIds(relayPid)) { + const child = await readShellProcessReadiness(childPid).catch(() => null) + if (isStale(scheduledGeneration)) { + return false + } + if ( + !child?.foreground || + !child.ttyPath || + child.ttyPath === options.slavePath || + !(await isExpectedShell(child.executablePath)) + ) { + continue + } + if ((await createPtySlaveLineEditorProbe(child.ttyPath)?.()) === 'line-editor') { + return true + } + } + return false + } + const probe = async (scheduledGeneration: number): Promise => { - if (disposed || scheduledGeneration !== generation) { + if (isStale(scheduledGeneration)) { return } const shellPid = options.getShellPid() - if (!shellPid || (await lineEditorProbe()) !== 'line-editor') { + if (!shellPid) { return } - if (disposed || scheduledGeneration !== generation) { + const outerState = await lineEditorProbe() + if (isStale(scheduledGeneration)) { return } - const [shell, expectedPath] = await Promise.all([ - readShellProcessReadiness(shellPid), - options.shellPath - ? resolveShellExecutablePath(options.shellPath, shellCwd, options.shellPathEnv) - : Promise.resolve(null) - ]) - if (disposed || scheduledGeneration !== generation) { + const shell = await readShellProcessReadiness(shellPid) + if (isStale(scheduledGeneration) || !shell?.foreground) { return } - if ( - !shell?.foreground || - !expectedShellName || - !expectedPath || - basename(shell.executablePath).toLowerCase() !== expectedShellName - ) { + const ready = (await isExpectedShell(shell.executablePath)) + ? outerState === 'line-editor' + : await isRelayedShellAtPrompt(shellPid, scheduledGeneration) + if (!ready || isStale(scheduledGeneration)) { return } - if (shell.executablePath !== expectedPath) { - // Profiles can exec another installed shell; accept only replacements found on this pane's PATH. - const installedPaths = await resolveInstalledShellExecutablePaths( - expectedShellName, - shellCwd, - options.shellPathEnv - ) - if ( - disposed || - scheduledGeneration !== generation || - !installedPaths.includes(shell.executablePath) - ) { - return - } - } disposed = true options.onPromptReady() } diff --git a/src/shared/shell-process-readiness.ts b/src/shared/shell-process-readiness.ts index dd6a325698d..65180d916bc 100644 --- a/src/shared/shell-process-readiness.ts +++ b/src/shared/shell-process-readiness.ts @@ -13,6 +13,7 @@ const DEFAULT_POSIX_EXEC_PATH = '/usr/bin:/bin' export type ShellProcessReadiness = { executablePath: string foreground: boolean + ttyPath: string | null } export function parseDarwinExecutablePath(stdout: string): string | null { @@ -45,18 +46,35 @@ export async function readShellProcessReadiness( ): Promise { const [executablePath, { stdout }] = await Promise.all([ readExecutablePath(pid), - execFile('ps', ['-p', String(pid), '-o', 'stat='], { + execFile('ps', ['-p', String(pid), '-o', 'stat=,tty='], { encoding: 'utf8', timeout: PROCESS_READINESS_TIMEOUT_MS, maxBuffer: PS_MAX_BUFFER_BYTES }) ]) - const status = stdout.trim() + const [status, ttyName] = stdout.trim().split(/\s+/) return status && executablePath - ? { executablePath: await realpath(executablePath), foreground: status.includes('+') } + ? { + executablePath: await realpath(executablePath), + foreground: status.includes('+'), + ttyPath: ttyName && ttyName !== '?' && ttyName !== '??' ? `/dev/${ttyName}` : null + } : null } +export async function listChildProcessIds(pid: number): Promise { + const { stdout } = await execFile('ps', ['-axo', 'pid=,ppid='], { + encoding: 'utf8', + timeout: PROCESS_READINESS_TIMEOUT_MS, + maxBuffer: PS_MAX_BUFFER_BYTES + }) + return stdout + .split('\n') + .map((line) => line.trim().split(/\s+/)) + .filter(([childPid, parentPid]) => childPid && parentPid === String(pid)) + .map(([childPid]) => Number(childPid)) +} + function shellExecutableCandidates( shellPath: string, cwd: string,