From 184f6dd8090c1afec313712660f396b22cb5698b Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 25 Jun 2026 11:33:45 +0000 Subject: [PATCH] style: bun run fix --- packages/coding-agent/CHANGELOG.md | 2 +- .../coding-agent/src/mcp/transports/stdio.ts | 5 ++ .../src/modes/controllers/input-controller.ts | 56 ++++++++++-------- .../test/input-controller-suspend.test.ts | 4 +- .../test/issue-3461-repro.test.ts | 59 ++++++++++--------- 5 files changed, 71 insertions(+), 55 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1df44f933..52b2a26c5 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed Ctrl+Z hanging the terminal after any tool call had run: the TUI tore down (`ui.stop()`) but the process kept running in `Sl+` state, leaving the user with a dead terminal recoverable only via `kill -9`. The embedded `brush-core` shell behind every bash tool call installs a tokio SIGTSTP listener on `Process::wait` (`crates/brush-core-vendored/src/sys/unix/signal.rs::tstp_signal_listener` → `tokio::signal::unix::signal(SIGTSTP)`); per tokio's contract, the first call for a SignalKind permanently replaces the kernel-default handler for the lifetime of the process. So the first bash invocation — even `/usr/bin/true` — silently overrode SIGTSTP's "stop" default, and `InputController.handleCtrlZ`'s subsequent `process.kill(0, "SIGTSTP")` was swallowed by tokio. The handler now sends `SIGSTOP` (uncatchable, unblockable, unignorable) to its own PID instead. Targeting self also leaves long-lived children (MCP stdio servers, the persistent brush native shell) running across the suspend — they're no longer frozen mid-IPC during a quick fg/bg detour ([#3461](https://github.com/can1357/oh-my-pi/issues/3461)). +- Fixed Ctrl+Z hanging the terminal after any tool call had run: the TUI tore down (`ui.stop()`) but the process kept running in `Sl+` state, leaving the user with a dead terminal recoverable only via `kill -9`. The embedded `brush-core` shell behind every bash tool call installs a tokio SIGTSTP listener on `Process::wait` (`crates/brush-core-vendored/src/sys/unix/signal.rs::tstp_signal_listener` → `tokio::signal::unix::signal(SIGTSTP)`); per tokio's contract, the first call for a SignalKind permanently replaces the kernel-default handler for the lifetime of the process. So the first bash invocation — even `/usr/bin/true` — silently overrode SIGTSTP's "stop" default, and `InputController.handleCtrlZ`'s subsequent `process.kill(0, "SIGTSTP")` was swallowed by tokio. The handler now sends `SIGSTOP` (uncatchable, unblockable, unignorable) to the foreground process group, so the kernel parks omp regardless of installed handlers and the shell sees the whole job stop even when omp runs behind a wrapper (`npx`, `pnpm exec`, `bunx`, …) or as one stage of a pipeline. MCP stdio servers now spawn detached into their own session — they're insulated both from terminal job-control signals (which used to stop their process trees and leave the JSONL read loop blocked on silent pipes) and from the new pgid=0 suspend itself ([#3461](https://github.com/can1357/oh-my-pi/issues/3461)). ## [16.1.19] - 2026-06-25 diff --git a/packages/coding-agent/src/mcp/transports/stdio.ts b/packages/coding-agent/src/mcp/transports/stdio.ts index c29f4e719..23b46f0e3 100644 --- a/packages/coding-agent/src/mcp/transports/stdio.ts +++ b/packages/coding-agent/src/mcp/transports/stdio.ts @@ -340,6 +340,10 @@ export class StdioTransport implements MCPTransport { platform: process.platform, }); + // Spawn in a new session (detached → setsid) so the MCP process tree has + // no controlling terminal. Otherwise terminal job-control signals (Ctrl+Z + // SIGTSTP, background-read SIGTTIN) can stop stdio servers such as + // chrome-devtools-mcp and leave our read loop blocked on silent pipes. this.#process = spawn({ cmd: spawnCommand.cmd, cwd, @@ -348,6 +352,7 @@ export class StdioTransport implements MCPTransport { stdout: "pipe", stderr: "pipe", windowsHide: spawnCommand.windowsHide, + detached: true, }); this.#connected = true; diff --git a/packages/coding-agent/src/modes/controllers/input-controller.ts b/packages/coding-agent/src/modes/controllers/input-controller.ts index 61cca702a..ca8724d07 100644 --- a/packages/coding-agent/src/modes/controllers/input-controller.ts +++ b/packages/coding-agent/src/modes/controllers/input-controller.ts @@ -926,35 +926,41 @@ export class InputController { this.ctx.ui.stop(); try { - // SIGSTOP — not SIGTSTP — and to our own PID, not the foreground process - // group. Two reasons: + // SIGSTOP — not SIGTSTP — to the foreground process group (pid=0). // - // 1. brush-core (the embedded shell behind every bash tool call) installs - // a tokio SIGTSTP listener on `Process::wait` to detect when its - // children have been stopped (`crates/brush-core-vendored/src/sys/ - // unix/signal.rs::tstp_signal_listener` → `tokio::signal::unix:: - // signal(SIGTSTP)`). Per tokio's documented contract, the first call - // for a given SignalKind permanently replaces the kernel-default - // handler for the lifetime of the process. So once the user has - // issued even one bash command — e.g. `/usr/bin/true` — SIGTSTP no - // longer stops omp: tokio swallows it and the TUI ends up torn down - // while the process keeps running with no live terminal (issue - // [#3461]). - // SIGSTOP cannot be caught, blocked, or ignored — it stops the - // process at the kernel regardless of what handlers are installed. + // SIGTSTP: brush-core (the embedded shell behind every bash tool call) + // installs a tokio SIGTSTP listener on `Process::wait` to detect when + // its children have been stopped (`crates/brush-core-vendored/src/sys/ + // unix/signal.rs::tstp_signal_listener` → `tokio::signal::unix:: + // signal(SIGTSTP)`). Per tokio's documented contract, the first call + // for a given SignalKind permanently replaces the kernel-default + // handler for the lifetime of the process. So once the user has + // issued even one bash command — e.g. `/usr/bin/true` — SIGTSTP no + // longer stops omp: tokio swallows it and the TUI ends up torn down + // while the process keeps running with no live terminal (issue + // [#3461]). SIGSTOP cannot be caught, blocked, or ignored, so the + // kernel stops the process regardless of installed handlers. // - // 2. Targeting our own PID instead of pgid=0 leaves long-lived children - // (MCP stdio servers, the persistent brush native shell) running - // while the agent is parked. Real terminal Ctrl+Z reaches the whole - // foreground group only because the kernel delivers it there; an - // internally-generated suspend has no reason to freeze the agent's - // IPC partners mid-request, and the parent shell still detects - // WIFSTOPPED via SIGCHLD and parks the job. - process.kill(process.pid, "SIGSTOP"); + // pid=0 (foreground process group, not just our PID): omp is not + // always the shell's direct child. Package-manager launchers (`npx`, + // `pnpm exec`, `bunx`, …) wait on the real CLI from a parent shim + // that shares omp's process group, and a `omp … | tee log` style + // pipeline puts a sibling foreground job member in the same group + // too. The shell sees the job as stopped only when its direct + // child / pipeline leader is stopped, so suspending only our PID + // leaves wrappers and pipeline peers running and the terminal + // hung — exactly the failure shape we're fixing. Stopping the whole + // group keeps the shell's job-control view consistent. Long-lived + // children that must survive the suspend (MCP stdio servers via + // the `detached: true` spawn in `mcp/transports/stdio.ts`, every + // brush external command via brush's per-child `setsid` in + // `crates/brush-core-vendored/src/commands.rs`) are already in + // their own sessions, so pgid=0 does not reach them. + process.kill(0, "SIGSTOP"); } catch (err) { // The runtime refused the signal (e.g. seccomp filter blocks SIGSTOP - // delivery to self). Tear the resume hook down and bring the TUI back - // so the user is not stranded on a frozen prompt. + // delivery to the process group). Tear the resume hook down and + // bring the TUI back so the user is not stranded on a frozen prompt. process.removeListener("SIGCONT", onResume); this.ctx.ui.start(); this.ctx.ui.requestRender(true); diff --git a/packages/coding-agent/test/input-controller-suspend.test.ts b/packages/coding-agent/test/input-controller-suspend.test.ts index d6525a13c..a0a9d120e 100644 --- a/packages/coding-agent/test/input-controller-suspend.test.ts +++ b/packages/coding-agent/test/input-controller-suspend.test.ts @@ -64,7 +64,7 @@ describe("InputController.handleCtrlZ", () => { expect(showError).not.toHaveBeenCalled(); }); - it("SIGSTOPs our own PID and registers a SIGCONT resume hook on POSIX (#3461)", () => { + it("SIGSTOPs the foreground process group and registers a SIGCONT resume hook on POSIX (#3461)", () => { setPlatform("linux"); const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true); const onceSpy = vi.spyOn(process, "once"); @@ -83,7 +83,7 @@ describe("InputController.handleCtrlZ", () => { expect(stopOrder).toBeLessThan(killOrder); expect(killSpy).toHaveBeenCalledTimes(1); - expect(killSpy).toHaveBeenCalledWith(process.pid, "SIGSTOP"); + expect(killSpy).toHaveBeenCalledWith(0, "SIGSTOP"); expect(ui.start).not.toHaveBeenCalled(); expect(showError).not.toHaveBeenCalled(); diff --git a/packages/coding-agent/test/issue-3461-repro.test.ts b/packages/coding-agent/test/issue-3461-repro.test.ts index 8ecbaf806..036e86c50 100644 --- a/packages/coding-agent/test/issue-3461-repro.test.ts +++ b/packages/coding-agent/test/issue-3461-repro.test.ts @@ -1,5 +1,5 @@ -import * as path from "node:path"; import { describe, expect, it } from "bun:test"; +import * as path from "node:path"; /** * Regression for https://github.com/can1357/oh-my-pi/issues/3461 @@ -14,28 +14,25 @@ import { describe, expect, it } from "bun:test"; * action, and `process.kill(0, "SIGTSTP")` from the Ctrl+Z handler became * a no-op. * - * The fix sends SIGSTOP (uncatchable) to our own PID instead. The unit - * test in `input-controller-suspend.test.ts` covers the JS handler's - * call shape; this file pins the runtime contract on the brush side so - * a brush upgrade or refactor that removes / gates the SIGTSTP listener - * forces a deliberate revisit of `handleCtrlZ` (we could go back to - * `process.kill(0, "SIGTSTP")` once the hijack is gone) instead of - * silently regressing behavior. + * The fix has two halves: + * + * - `handleCtrlZ` sends SIGSTOP (uncatchable) to the foreground process + * group, so the kernel parks omp regardless of installed handlers and the + * parent shell sees the whole job stop even when omp runs behind a wrapper + * (`npx`, `pnpm exec`, `bunx`, …) or as one stage of a pipeline. + * - MCP stdio servers spawn detached, so terminal job-control signals cannot + * stop their process trees and leave the JSONL read loop blocked on silent + * pipes — and so the pgid=0 suspend above doesn't reach them either. + * The unit test in `input-controller-suspend.test.ts` covers the JS handler's + * call shape; this file pins the runtime contract on the brush/MCP side so + * refactors force a deliberate revisit instead of silently regressing behavior. */ describe("issue #3461 — Ctrl+Z hangs after a command has been run", () => { const packageDir = path.resolve(import.meta.dir, ".."); - const brushUnixSignal = path.resolve( - packageDir, - "../../crates/brush-core-vendored/src/sys/unix/signal.rs", - ); - const brushProcesses = path.resolve( - packageDir, - "../../crates/brush-core-vendored/src/processes.rs", - ); - const inputController = path.resolve( - packageDir, - "src/modes/controllers/input-controller.ts", - ); + const brushUnixSignal = path.resolve(packageDir, "../../crates/brush-core-vendored/src/sys/unix/signal.rs"); + const brushProcesses = path.resolve(packageDir, "../../crates/brush-core-vendored/src/processes.rs"); + const inputController = path.resolve(packageDir, "src/modes/controllers/input-controller.ts"); + const mcpStdioTransport = path.resolve(packageDir, "src/mcp/transports/stdio.ts"); it("brush-core installs a tokio SIGTSTP listener on every Process::wait", async () => { const signalSrc = await Bun.file(brushUnixSignal).text(); @@ -49,14 +46,22 @@ describe("issue #3461 — Ctrl+Z hangs after a command has been run", () => { expect(processesSrc).toContain("tstp_signal_listener"); }); - it("handleCtrlZ sends SIGSTOP — not SIGTSTP — to our own PID, defeating the brush hijack", async () => { + it("handleCtrlZ sends SIGSTOP to the foreground process group, defeating the brush hijack and covering wrappers/pipelines", async () => { const src = await Bun.file(inputController).text(); - // `process.kill(0, "SIGTSTP")` is the broken call shape; it must not - // reappear in the Ctrl+Z handler. + // The original broken shape must not return. expect(src).not.toMatch(/process\.kill\(\s*0\s*,\s*["']SIGTSTP["']\s*\)/); - // SIGSTOP to our own PID is the only correct shape: SIGSTOP cannot be - // caught/blocked/ignored, and targeting self leaves child MCP / native - // shell processes running across the suspend. - expect(src).toMatch(/process\.kill\(\s*process\.pid\s*,\s*["']SIGSTOP["']\s*\)/); + // Self-only SIGSTOP was the v1 of this fix; it leaves wrapper/pipeline + // peers in the same process group running and the shell never sees the + // job stop. The handler now targets pgid=0. + expect(src).not.toMatch(/process\.kill\(\s*process\.pid\s*,\s*["']SIGSTOP["']\s*\)/); + // pgid=0 SIGSTOP is the only correct shape: uncatchable, and reaches + // every process the shell considers part of the foreground job. + expect(src).toMatch(/process\.kill\(\s*0\s*,\s*["']SIGSTOP["']\s*\)/); + }); + + it("MCP stdio servers spawn detached so terminal job-control signals cannot stop them", async () => { + const src = await Bun.file(mcpStdioTransport).text(); + expect(src).toMatch(/detached:\s*true/); + expect(src).toContain("no controlling terminal"); }); });