From 90c4726baefe21d3fedaac40d8d0bf81330a0722 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 14 Jul 2026 20:17:03 +0000 Subject: [PATCH] fix(launch): passed Windows PTY arguments directly Added a direct-argv PTY entry point and used it for Windows launch sessions so portable-pty no longer re-quotes cmd.exe command text. Rejected direct .bat and .cmd applications with guidance to use cmd.exe /c. Fixes #5416 --- crates/pi-natives/src/pty.rs | 153 +++++++++++++++------ packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/src/launch/broker.ts | 59 ++++---- packages/natives/CHANGELOG.md | 4 + packages/natives/native/index.d.ts | 24 +++- packages/natives/test/native.test.ts | 28 ++++ 6 files changed, 202 insertions(+), 70 deletions(-) diff --git a/crates/pi-natives/src/pty.rs b/crates/pi-natives/src/pty.rs index c0354d03f..f07912909 100644 --- a/crates/pi-natives/src/pty.rs +++ b/crates/pi-natives/src/pty.rs @@ -44,6 +44,27 @@ pub struct PtyStartOptions<'env> { pub shell: Option, } +/// Options for running an executable and argument vector in a PTY session. +#[napi(object)] +pub struct PtyArgvStartOptions<'env> { + /// Executable name or path. + pub application: String, + /// Arguments passed directly to the executable. + pub args: Vec, + /// Working directory for command execution. + pub cwd: Option, + /// Environment variables for this command. + pub env: Option>, + /// Timeout in milliseconds before cancelling. + pub timeout_ms: Option, + /// Abort signal for cancelling the operation. + pub signal: Option>, + /// PTY column count. + pub cols: Option, + /// PTY row count. + pub rows: Option, +} + /// Result of a PTY command run. #[napi(object)] pub struct PtyRunResult { @@ -55,14 +76,19 @@ pub struct PtyRunResult { pub timed_out: bool, } +#[derive(Clone)] +enum PtyCommand { + Shell { command: String, shell: Option }, + Argv { application: String, args: Vec }, +} + #[derive(Clone)] struct PtyRunConfig { - command: String, + command: PtyCommand, cwd: Option, env: Option>, cols: u16, rows: u16, - shell: Option, } enum ReaderEvent { @@ -106,7 +132,7 @@ impl PtySession { Self { core: Arc::new(Mutex::new(None)) } } - /// Start a PTY command and stream output chunks via callback. + /// Start a shell command and stream output chunks via callback. #[napi] pub fn start<'env>( &self, @@ -116,40 +142,33 @@ impl PtySession { on_chunk: Option>, ) -> Result> { let run_config = PtyRunConfig { - command: options.command, + command: PtyCommand::Shell { command: options.command, shell: options.shell }, cwd: options.cwd, env: options.env, cols: options.cols.unwrap_or(120).clamp(20, 400), rows: options.rows.unwrap_or(40).clamp(5, 200), - shell: options.shell, }; - let ct = task::CancelToken::new(options.timeout_ms, options.signal); - let core = Arc::clone(&self.core); + self.start_config(env, run_config, options.timeout_ms, options.signal, on_chunk) + } - // Register control channel synchronously so write()/kill() work immediately. - let (control_tx, control_rx) = flume::unbounded::(); - { - let mut guard = core.lock(); - if guard.is_some() { - return Err(Error::from_reason("PTY session already running")); - } - *guard = Some(PtySessionCore { control_tx }); - } - task::future(env, "pty.start", async move { - let run_result = - tokio::task::spawn_blocking(move || run_pty_sync(run_config, on_chunk, control_rx, ct)) - .await; - - // Always clear core regardless of result - let mut guard = core.lock(); - *guard = None; - drop(guard); - - match run_result { - Ok(inner) => inner, - Err(err) => Err(Error::from_reason(format!("PTY execution task failed: {err}"))), - } - }) + /// Start an executable with separate arguments and stream output chunks via + /// callback. + #[napi] + pub fn start_argv<'env>( + &self, + env: &'env Env, + options: PtyArgvStartOptions<'env>, + #[napi(ts_arg_type = "((error: Error | null, chunk: string) => void) | undefined | null")] + on_chunk: Option>, + ) -> Result> { + let run_config = PtyRunConfig { + command: PtyCommand::Argv { application: options.application, args: options.args }, + cwd: options.cwd, + env: options.env, + cols: options.cols.unwrap_or(120).clamp(20, 400), + rows: options.rows.unwrap_or(40).clamp(5, 200), + }; + self.start_config(env, run_config, options.timeout_ms, options.signal, on_chunk) } /// Write raw input bytes to PTY stdin. @@ -175,6 +194,42 @@ impl PtySession { } impl PtySession { + fn start_config<'env>( + &self, + env: &'env Env, + run_config: PtyRunConfig, + timeout_ms: Option, + signal: Option>, + on_chunk: Option>, + ) -> Result> { + let ct = task::CancelToken::new(timeout_ms, signal); + let core = Arc::clone(&self.core); + + // Register control channel synchronously so write()/kill() work immediately. + let (control_tx, control_rx) = flume::unbounded::(); + { + let mut guard = core.lock(); + if guard.is_some() { + return Err(Error::from_reason("PTY session already running")); + } + *guard = Some(PtySessionCore { control_tx }); + } + task::future(env, "pty.start", async move { + let run_result = + tokio::task::spawn_blocking(move || run_pty_sync(run_config, on_chunk, control_rx, ct)) + .await; + + let mut guard = core.lock(); + *guard = None; + drop(guard); + + match run_result { + Ok(inner) => inner, + Err(err) => Err(Error::from_reason(format!("PTY execution task failed: {err}"))), + } + }) + } + fn send_control(&self, message: ControlMessage) -> Result<()> { let guard = self.core.lock(); let core = guard @@ -249,19 +304,29 @@ fn run_pty_sync( .map_err(|err| Error::from_reason(format!("Failed to open PTY: {err}")))? }; - let shell = config.shell.as_deref().unwrap_or("sh"); - let mut cmd = CommandBuilder::new(shell); - // Use shell-appropriate command execution flags - let lower = shell.to_lowercase(); - if lower.ends_with("cmd.exe") || lower.ends_with("cmd") { - cmd.arg("/c"); - } else if lower.contains("powershell") || lower.contains("pwsh") { - cmd.arg("-Command"); - } else { - // sh/bash/zsh/fish etc. - cmd.arg("-lc"); - } - cmd.arg(&config.command); + let mut cmd = match config.command { + PtyCommand::Shell { command, shell } => { + let shell = shell.as_deref().unwrap_or("sh"); + let mut cmd = CommandBuilder::new(shell); + let lower = shell.to_lowercase(); + if lower.ends_with("cmd.exe") || lower.ends_with("cmd") { + cmd.arg("/c"); + } else if lower.contains("powershell") || lower.contains("pwsh") { + cmd.arg("-Command"); + } else { + cmd.arg("-lc"); + } + cmd.arg(command); + cmd + }, + PtyCommand::Argv { application, args } => { + let mut cmd = CommandBuilder::new(application); + for arg in args { + cmd.arg(arg); + } + cmd + }, + }; if let Some(cwd) = config.cwd.as_ref() { cmd.cwd(cwd); } diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2acfde738..cca64b703 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed the `launch` tool failing to start Windows executables because PTY commands double-escaped the application and arguments ([#5416](https://github.com/can1357/oh-my-pi/issues/5416)). + ## [16.5.1] - 2026-07-14 ### Changed diff --git a/packages/coding-agent/src/launch/broker.ts b/packages/coding-agent/src/launch/broker.ts index 9511ae9a9..e8212b8a8 100644 --- a/packages/coding-agent/src/launch/broker.ts +++ b/packages/coding-agent/src/launch/broker.ts @@ -84,9 +84,6 @@ interface DaemonLogRead { function quoteShellArg(value: string): string { return `'${value.replaceAll("'", `'\\''`)}'`; } -function quoteCmdArg(value: string): string { - return `"${value.replaceAll('"', '""')}"`; -} function terminalState(state: DaemonSnapshot["state"]): boolean { return state === "exited" || state === "failed"; @@ -434,6 +431,9 @@ class DaemonBroker { if (spec.detached && spec.pty) { throw new Error("A detached daemon cannot allocate a PTY"); } + if (process.platform === "win32" && [".bat", ".cmd"].includes(path.extname(spec.application).toLowerCase())) { + throw new Error('Windows batch files require application "cmd.exe" with the batch path after "/c"'); + } const existing = this.#records.get(spec.name); if (existing) await this.#refreshDetached(existing); if (existing && !terminalState(existing.snapshot.state)) { @@ -520,38 +520,47 @@ class DaemonBroker { } async #launchPty(record: ManagedDaemon, generation: number): Promise { - const pidPath = path.join(record.dir, "process.pid"); - await fs.rm(pidPath, { force: true }); - const argv = [record.spec.application, ...record.spec.args]; - const command = - process.platform === "win32" - ? argv.map(quoteCmdArg).join(" ") - : [`printf '%s' "$$" > ${quoteShellArg(pidPath)}`, `exec ${argv.map(quoteShellArg).join(" ")}`].join("; "); const session = new PtySession(); record.pty = session; - const shell = process.platform === "win32" ? process.env.COMSPEC : process.env.SHELL; - void session - .start( + const options = { + cwd: record.spec.cwd, + env: workerEnvFromParent({ TERM: "xterm-256color", ...record.spec.env }), + cols: DAEMON_PTY_COLUMNS, + rows: DAEMON_PTY_ROWS, + }; + const onChunk = (error: Error | null, chunk: string): void => { + if (generation !== record.generation) return; + if (error) record.log?.append(`PTY output error: ${error.message}\n`); + if (chunk) this.#onOutput(record, generation, chunk); + }; + let run: Promise; + if (process.platform === "win32") { + run = session.startArgv( { - command, - cwd: record.spec.cwd, - env: workerEnvFromParent({ TERM: "xterm-256color", ...record.spec.env }), - cols: DAEMON_PTY_COLUMNS, - rows: DAEMON_PTY_ROWS, - shell, + application: record.spec.application, + args: record.spec.args, + ...options, }, - (error, chunk) => { - if (generation !== record.generation) return; - if (error) record.log?.append(`PTY output error: ${error.message}\n`); - if (chunk) this.#onOutput(record, generation, chunk); - }, - ) + onChunk, + ); + } else { + const pidPath = path.join(record.dir, "process.pid"); + await fs.rm(pidPath, { force: true }); + const argv = [record.spec.application, ...record.spec.args]; + const command = [ + `printf '%s' "$$" > ${quoteShellArg(pidPath)}`, + `exec ${argv.map(quoteShellArg).join(" ")}`, + ].join("; "); + run = session.start({ command, shell: process.env.SHELL, ...options }, onChunk); + } + void run .then(result => this.#onPtyExit(record, generation, result)) .catch(error => this.#settle(record, generation, undefined, error instanceof Error ? error.message : String(error)), ); if (process.platform === "win32") return; + const pidPath = path.join(record.dir, "process.pid"); const deadline = Date.now() + 5_000; const pidFile = Bun.file(pidPath); while (Date.now() < deadline && generation === record.generation) { diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index 7f715115d..9a9f7cd1f 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed Windows PTY callers being forced through shell command re-quoting by adding direct executable-and-argument launching ([#5416](https://github.com/can1357/oh-my-pi/issues/5416)). + ## [16.4.6] - 2026-07-12 ### Added diff --git a/packages/natives/native/index.d.ts b/packages/natives/native/index.d.ts index aa4488989..1e8df42e4 100644 --- a/packages/natives/native/index.d.ts +++ b/packages/natives/native/index.d.ts @@ -84,8 +84,10 @@ export declare class Process { /** Stateful PTY session for interactive stdin/stdout passthrough. */ export declare class PtySession { constructor() - /** Start a PTY command and stream output chunks via callback. */ + /** Start a shell command and stream output chunks via callback. */ start(options: PtyStartOptions, onChunk?: ((error: Error | null, chunk: string) => void) | undefined | null): Promise + /** Start an executable with separate arguments and stream output chunks via callback. */ + startArgv(options: PtyArgvStartOptions, onChunk?: ((error: Error | null, chunk: string) => void) | undefined | null): Promise /** Write raw input bytes to PTY stdin. */ write(data: string): void /** Resize the active PTY. */ @@ -1250,6 +1252,26 @@ export interface ProcessWaitOptions { signal?: unknown } +/** Options for running an executable and argument vector in a PTY session. */ +export interface PtyArgvStartOptions { + /** Executable name or path. */ + application: string + /** Arguments passed directly to the executable. */ + args: Array + /** Working directory for command execution. */ + cwd?: string + /** Environment variables for this command. */ + env?: Record + /** Timeout in milliseconds before cancelling. */ + timeoutMs?: number + /** Abort signal for cancelling the operation. */ + signal?: unknown + /** PTY column count. */ + cols?: number + /** PTY row count. */ + rows?: number +} + /** Result of a PTY command run. */ export interface PtyRunResult { /** Exit code when the command completes. */ diff --git a/packages/natives/test/native.test.ts b/packages/natives/test/native.test.ts index 6c9562793..57353166d 100644 --- a/packages/natives/test/native.test.ts +++ b/packages/natives/test/native.test.ts @@ -613,6 +613,34 @@ describe("pi-natives", () => { }); describe("pty", () => { + it("passes executable arguments without shell quoting", async () => { + const scriptPath = path.join(testDir, "pty-argv.ts"); + const expected = ["argument with spaces", 'quote"inside', "backslash\\end"]; + await Bun.write(scriptPath, 'process.stdout.write(JSON.stringify(process.argv.slice(2)) + "\\n");\n'); + const session = new PtySession(); + let output = ""; + let callbackError: Error | null = null; + const result = await session.startArgv( + { + application: process.execPath, + args: [scriptPath, ...expected], + cwd: testDir, + timeoutMs: 5_000, + cols: 80, + rows: 24, + }, + (error, chunk) => { + callbackError = error; + output += chunk; + }, + ); + + expect(callbackError).toBeNull(); + expect(result.exitCode).toBe(0); + expect(result.timedOut).toBeFalse(); + expect(JSON.parse(output.trim())).toEqual(expected); + }); + it("should time out detached background workloads without hanging", async () => { if (process.platform === "win32" || !Bun.which("bash")) { return;