Merge PR #5520: fix(launch): pass Windows PTY arguments directly (@roboomp)

This commit is contained in:
can1357
2026-07-14 23:11:07 +02:00
6 changed files with 201 additions and 70 deletions
+109 -44
View File
@@ -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);
}
+3
View File
@@ -39,6 +39,9 @@
- Fixed confusing launch start/wait results when readiness timed out with the log pattern already matched (readiness needs log AND port): the result printed a contradictory `Ready: <match>` next to `Readiness timed out` without naming the failing condition. Daemon snapshots now carry the unmet conditions (`readyPending`), and start/wait results state exactly what never happened (e.g. `port 3100 on 127.0.0.1 never accepted connections`); the TUI shows a `waiting on port` badge on starting daemons
- Fixed the in-process `stat` builtin mangling BSD-style invocations like `stat -f "%Sm %N" file` (macOS muscle memory): GNU `-f` means `--file-system`, so the format string was treated as a file operand — printing filesystem info for the real operands and erroring with `cannot read file system information for '%Sm %N'`. A `-f` whose format value contains `%` is now detected as BSD syntax and translated to the GNU equivalent (`%Sm`→`%y`, `%N`→`%n`, `%z`→`%s`, epoch/`S`-form times, owner/group/permission and `H`/`L` sub-field directives, `-L`/`-n`/`-q`/`-F` flag clusters, with `%n`/`%t` as literal newline/tab); directives with no GNU counterpart fail with a clear `unsupported BSD format directive` error
- Fixed the remaining GNU-flavored shell builtins that broke under macOS/BSD muscle memory, using the same unambiguous-detection approach as the `stat` fix (only invocations that are invalid or nonsensical under GNU semantics are reinterpreted; unsupported BSD forms fail loudly instead of producing wrong output): `date -r <epoch>` formats the epoch when no such file exists (GNU `-r FILE` mtime preserved), signed `date -v±N<unit>` adjustments translate to `-d` relative dates and `-j` is accepted (`-j -f` strptime parse mode and field-set `-v` error clearly); `sed -i '' 's/…/…/' file` drops the BSD empty backup-suffix token instead of treating it as the script; `mktemp -t prefix` without X's creates `$TMPDIR/prefix.XXXXXXXXXX` (the GNU `too few X's` error path); `tail -r` reverses input by delegating to `tac` (with `-n`/`-c`/`-f` combinations erroring clearly); `find -E` maps to `-regextype posix-extended` ahead of the expression; `base64 -D` decodes as an alias of `-d`; and `ln -sfh` works via a `-h` alias of `--no-dereference` (clap's `-h` help short is dropped to match real GNU/BSD ln; `--help` unchanged)
### 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
+34 -25
View File
@@ -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) {
+4
View File
@@ -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
+23 -1
View File
@@ -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. */
+28
View File
@@ -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;