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
This commit is contained in:
+109
-44
@@ -44,6 +44,27 @@ pub struct PtyStartOptions<'env> {
|
||||
pub shell: Option<String>,
|
||||
}
|
||||
|
||||
/// 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<String>,
|
||||
/// Working directory for command execution.
|
||||
pub cwd: Option<String>,
|
||||
/// Environment variables for this command.
|
||||
pub env: Option<HashMap<String, String>>,
|
||||
/// Timeout in milliseconds before cancelling.
|
||||
pub timeout_ms: Option<u32>,
|
||||
/// Abort signal for cancelling the operation.
|
||||
pub signal: Option<Unknown<'env>>,
|
||||
/// PTY column count.
|
||||
pub cols: Option<u16>,
|
||||
/// PTY row count.
|
||||
pub rows: Option<u16>,
|
||||
}
|
||||
|
||||
/// 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<String> },
|
||||
Argv { application: String, args: Vec<String> },
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
struct PtyRunConfig {
|
||||
command: String,
|
||||
command: PtyCommand,
|
||||
cwd: Option<String>,
|
||||
env: Option<HashMap<String, String>>,
|
||||
cols: u16,
|
||||
rows: u16,
|
||||
shell: Option<String>,
|
||||
}
|
||||
|
||||
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<ThreadsafeFunction<String>>,
|
||||
) -> Result<PromiseRaw<'env, PtyRunResult>> {
|
||||
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::<ControlMessage>();
|
||||
{
|
||||
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<ThreadsafeFunction<String>>,
|
||||
) -> Result<PromiseRaw<'env, PtyRunResult>> {
|
||||
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<u32>,
|
||||
signal: Option<Unknown<'env>>,
|
||||
on_chunk: Option<ThreadsafeFunction<String>>,
|
||||
) -> Result<PromiseRaw<'env, PtyRunResult>> {
|
||||
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::<ControlMessage>();
|
||||
{
|
||||
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);
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<void> {
|
||||
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<PtyRunResult>;
|
||||
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) {
|
||||
|
||||
@@ -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
|
||||
|
||||
Vendored
+23
-1
@@ -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<PtyRunResult>
|
||||
/** 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<PtyRunResult>
|
||||
/** 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<string>
|
||||
/** Working directory for command execution. */
|
||||
cwd?: string
|
||||
/** Environment variables for this command. */
|
||||
env?: Record<string, string>
|
||||
/** 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. */
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user