diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 9fe2494f3..31b2c5706 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -25,6 +25,10 @@ - Updated status event log to prioritize the most recent entries in the display window - Updated the snapcompact shape preview transcript to use the compact scope format shown to models during compaction. +### Fixed + +- Fixed eval cells launching interactive subprocesses in OMP's terminal session, which could steal the foreground process group and suspend the conversation with `zsh: suspended (tty input)` + ### Removed - Removed the `--downshift-boomerang` feature and its associated configuration setting diff --git a/packages/coding-agent/src/cli.ts b/packages/coding-agent/src/cli.ts index 20ad9038c..60ec786b8 100755 --- a/packages/coding-agent/src/cli.ts +++ b/packages/coding-agent/src/cli.ts @@ -108,6 +108,7 @@ const TINY_WORKER_ARG = "__omp_worker_tiny_inference"; const STATS_SYNC_WORKER_ARG = "__omp_worker_stats_sync"; const TAB_WORKER_ARG = "__omp_worker_tab"; const JS_EVAL_WORKER_ARG = "__omp_worker_js_eval"; +const JS_EVAL_PROCESS_ARG = "__omp_worker_js_eval_process"; const STT_WORKER_ARG = "__omp_worker_stt"; const TTS_WORKER_ARG = "__omp_worker_tts"; const MNEMOPI_EMBED_WORKER_ARG = "__omp_worker_mnemopi_embed"; @@ -156,6 +157,11 @@ async function runWorkerEntrypoint(arg: string | undefined): Promise { await import("./eval/js/worker-entry"); return true; } + if (arg === JS_EVAL_PROCESS_ARG) { + const { startJsEvalProcess } = await import("./eval/js/process-entry"); + await runIpcSubprocessWorker(startJsEvalProcess); + return true; + } if (arg === STT_WORKER_ARG) { const { startSttWorker } = await import("./stt/asr-worker"); await runIpcSubprocessWorker(startSttWorker); diff --git a/packages/coding-agent/src/eval/__tests__/js-context-manager.test.ts b/packages/coding-agent/src/eval/__tests__/js-context-manager.test.ts index fc0e39ea5..43d89645d 100644 --- a/packages/coding-agent/src/eval/__tests__/js-context-manager.test.ts +++ b/packages/coding-agent/src/eval/__tests__/js-context-manager.test.ts @@ -2,7 +2,11 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; import { TempDir } from "@oh-my-pi/pi-utils"; import { Settings } from "../../config/settings"; import type { ToolSession } from "../../tools"; -import { disposeAllVmContexts, setWorkerCloseTimeoutMsForTests } from "../js/context-manager"; +import { + disposeAllVmContexts, + setJsEvalWorkerThreadForTests, + setWorkerCloseTimeoutMsForTests, +} from "../js/context-manager"; import { executeJs } from "../js/executor"; const originalWorker = globalThis.Worker; @@ -181,7 +185,9 @@ function installFakeWorker(stats: FakeWorkerStats, behavior: FakeWorkerBehavior) describe("JavaScript eval worker lifecycle", () => { let restoreCloseTimeoutMs = 0; + let restoreWorkerThread = false; beforeEach(() => { + restoreWorkerThread = setJsEvalWorkerThreadForTests(true); // Shrink the graceful-close grace period so the "close acked but the worker // never exits -> force terminate" contract is proven without a real 1s wait. restoreCloseTimeoutMs = setWorkerCloseTimeoutMsForTests(1); @@ -197,6 +203,7 @@ describe("JavaScript eval worker lifecycle", () => { writable: true, value: originalWorker, }); + setJsEvalWorkerThreadForTests(restoreWorkerThread); }); it("exits a real worker on graceful close even with ref'ed user handles", async () => { @@ -289,3 +296,37 @@ describe("JavaScript eval worker lifecycle", () => { expect(stats.terminateCalls).toBe(1); }); }); + +describe.skipIf(process.platform === "win32")("JavaScript eval process isolation", () => { + afterEach(async () => { + await disposeAllVmContexts(); + }); + + it("runs spawned commands under an isolated POSIX session", async () => { + using tempDir = TempDir.createSync("@omp-js-process-isolation-"); + const session = makeSession(tempDir.path()); + const evalSessionId = `js-isolation:${crypto.randomUUID()}`; + const result = await executeJs( + [ + `const child = Bun.spawn(["/bin/sh", "-c", 'sid=$(ps -o sid= -p $$); printf "%s %s\\n" "$sid" "$PPID"'], { stdout: "pipe" });`, + "return await new Response(child.stdout).text();", + ].join("\n"), + { cwd: tempDir.path(), sessionId: evalSessionId, session }, + ); + const [sessionId, parentProcessId] = result.output.trim().split(/\s+/).map(Number); + expect(parentProcessId).not.toBe(process.pid); + expect(sessionId).toBe(parentProcessId); + + await executeJs("var saved = 41; function increment(value) { return value + 1; }", { + cwd: tempDir.path(), + sessionId: evalSessionId, + session, + }); + const reused = await executeJs("return increment(saved);", { + cwd: tempDir.path(), + sessionId: evalSessionId, + session, + }); + expect(reused.output.trim()).toBe("42"); + }); +}); diff --git a/packages/coding-agent/src/eval/__tests__/kernel-spawn.test.ts b/packages/coding-agent/src/eval/__tests__/kernel-spawn.test.ts index 0d7cdd4cc..5e58efaf0 100644 --- a/packages/coding-agent/src/eval/__tests__/kernel-spawn.test.ts +++ b/packages/coding-agent/src/eval/__tests__/kernel-spawn.test.ts @@ -3,9 +3,21 @@ import { __resetWindowsConsoleProbeCache, consoleAttachedViaTTY, hostHasInheritableConsole, + shouldDetachKernel, shouldHideKernelWindow, } from "../py/spawn-options"; +describe("shouldDetachKernel", () => { + it("starts POSIX kernels in a new session", () => { + expect(shouldDetachKernel("darwin")).toBe(true); + expect(shouldDetachKernel("linux")).toBe(true); + }); + + it("leaves Windows console inheritance to windowsHide", () => { + expect(shouldDetachKernel("win32")).toBe(false); + }); +}); + /** * `shouldHideKernelWindow` decides whether the long-lived Python kernel * subprocess is spawned with `windowsHide: true`. On Windows, Bun maps that diff --git a/packages/coding-agent/src/eval/jl/kernel.ts b/packages/coding-agent/src/eval/jl/kernel.ts index 290837ac9..3ada97e16 100644 --- a/packages/coding-agent/src/eval/jl/kernel.ts +++ b/packages/coding-agent/src/eval/jl/kernel.ts @@ -13,7 +13,7 @@ import { $ } from "bun"; import { Settings } from "../../config/settings"; import { BaseKernel, getRemainingTimeMs, type KernelStartOptions } from "../kernel-base"; import type { KernelDisplayOutput } from "../py/display"; -import { hostHasInheritableConsole, shouldHideKernelWindow } from "../py/spawn-options"; +import { hostHasInheritableConsole, shouldDetachKernel, shouldHideKernelWindow } from "../py/spawn-options"; import { JULIA_PRELUDE } from "./prelude"; import RUNNER_SCRIPT from "./runner.jl" with { type: "text" }; import { @@ -187,6 +187,7 @@ export class JuliaKernel extends BaseKernel { [runtime.juliaPath, "--startup-file=no", "--history-file=no", "--color=no", "--project=@.", scriptPath], { cwd: options.cwd, + detached: shouldDetachKernel(process.platform), env: spawnEnv, stdin: "pipe", stdout: "pipe", diff --git a/packages/coding-agent/src/eval/js/context-manager.ts b/packages/coding-agent/src/eval/js/context-manager.ts index 3ad19f690..3bff82694 100644 --- a/packages/coding-agent/src/eval/js/context-manager.ts +++ b/packages/coding-agent/src/eval/js/context-manager.ts @@ -1,6 +1,14 @@ import { logger, Snowflake, workerHostEntry } from "@oh-my-pi/pi-utils"; +import { + createWorkerHandle, + createWorkerSubprocess, + resolveWorkerSpawnCmd, + workerEnvFromParent, +} from "../../subprocess/worker-client"; import type { ToolSession } from "../../tools"; import { ToolAbortError, ToolError } from "../../tools/tool-errors"; +import { safeSend as safeSendIpc } from "../../utils/ipc"; +import { shouldDetachKernel } from "../py/spawn-options"; import { callSessionTool, type JsStatusEvent } from "./tool-bridge"; import { WorkerCore } from "./worker-core"; // Coding-agent binary/bundle workers route through the CLI entrypoint with a @@ -24,7 +32,7 @@ export interface VmRunState { } interface WorkerHandle { - mode: "worker" | "inline"; + mode: "process" | "worker" | "inline"; send(msg: WorkerInbound): void; onMessage(handler: (msg: WorkerOutbound) => void): () => void; onError(handler: (error: Error) => void): () => void; @@ -57,16 +65,18 @@ const resettingSessions = new Map>(); // Worker startup (module-graph import + WorkerCore construction) is infrastructure // cost, not user compute. Floor it independently of Bun's 5s default per-test timeout // so a slow cold-start under load isn't aborted mid-init — terminating a still- -// initializing Bun worker triggers the same kind of terminate-race that motivates +// initializing eval runtime triggers the same kind of terminate-race that motivates // avoiding `vm.runInContext` (see shared/indirect-eval.ts), here surfacing as a // SIGILL/SIGSEGV. Callers that pass a larger per-cell budget still dominate. const WORKER_INIT_TIMEOUT_MS = 15_000; const WORKER_CLOSE_TIMEOUT_MS = 1_000; +const JS_EVAL_PROCESS_ARG = "__omp_worker_js_eval_process"; // Active graceful-close grace period before a worker that ack'd `close` but never // emitted its `close` event is force-terminated. Defaults to the production floor; // tests override it (and restore it) to exercise the close-timeout -> terminate // path without a real wall-clock wait. let workerCloseTimeoutMs: number = WORKER_CLOSE_TIMEOUT_MS; +let useWorkerThreadForTests = false; /** * Test-only seam: override the graceful-close grace period (ms). Returns the @@ -79,6 +89,13 @@ export function setWorkerCloseTimeoutMsForTests(ms: number): number { return previous; } +/** Test-only seam for the legacy Worker lifecycle mocks. */ +export function setJsEvalWorkerThreadForTests(enabled: boolean): boolean { + const previous = useWorkerThreadForTests; + useWorkerThreadForTests = enabled; + return previous; +} + export async function executeInVmContext(options: { sessionKey: string; sessionId: string; @@ -144,9 +161,9 @@ export async function disposeAllVmContexts(): Promise { } /** - * Smoke probe: spawn the JS eval worker through the worker-host entry and prove - * it answers the `init` handshake on a real worker thread (not the inline - * fallback). Catches the silent worker-load and init-message-drop regressions + * Smoke probe: spawn the JS evaluator through the worker-host entry and prove + * it answers the `init` handshake in a real isolated subprocess (not the inline + * fallback). Catches silent process-load and init-message regressions * that otherwise strand every cell on the init timeout in a distribution build — * the failure mode that motivated `installWorkerInbox`. Wired into * `omp --smoke-test` so binary / source / tarball installs all exercise it. @@ -163,8 +180,8 @@ export async function smokeTestJsEvalWorker(): Promise { }; try { await initWorker(session, { cwd: process.cwd(), sessionId: "smoke" }, WORKER_INIT_TIMEOUT_MS); - if (worker.mode !== "worker") { - throw new Error("JS eval worker smoke fell back to the inline worker (real worker failed to start)"); + if (worker.mode !== "process") { + throw new Error("JS eval worker smoke fell back from the isolated subprocess"); } } finally { await worker.terminate().catch(() => undefined); @@ -237,10 +254,8 @@ async function acquireSession(sessionKey: string, snapshot: SessionSnapshot, tim if (starting) return await starting; const startup = (async (): Promise => { - // The message listener must be attached synchronously after `new Worker`: - // Bun drops messages posted before a listener exists, and WorkerCore emits - // `ready` from its constructor on load. `spawnJsWorker` + `initWorker` run with - // no intervening await, so `ready` can never race the attach. + // Attach the message listener before sending init. Both Bun Worker messages + // and subprocess IPC can arrive immediately after the evaluator loads. const worker = spawnJsWorker(); const session: JsSession = { sessionKey, @@ -256,8 +271,8 @@ async function acquireSession(sessionKey: string, snapshot: SessionSnapshot, tim try { await initWorker(session, snapshot, readyTimeoutMs); } catch (error) { - // Worker-thread crash/load failures surface asynchronously via the worker - // `error` event — after `spawnJsWorker`'s synchronous try/catch already + // Runtime crash/load failures surface asynchronously via the worker error + // callback — after `spawnJsWorker`'s synchronous try/catch already // returned — so the only signal is the rejected handshake. Retry on the // inline worker so a broken module graph fails fast instead of stalling // every cell on the init timeout and then dying with exitCode 1. @@ -480,6 +495,16 @@ async function raceWithTimeout(promise: Promise, timeoutMs: number, reason } function spawnJsWorker(): WorkerHandle { + if (!useWorkerThreadForTests) { + try { + return spawnJsProcess(); + } catch (err) { + logger.warn("JS eval subprocess spawn failed; using inline JS eval worker (no sync-loop guard)", { + error: err instanceof Error ? err.message : String(err), + }); + return spawnInlineWorker(); + } + } try { const hostEntry = workerHostEntry(); const worker = hostEntry @@ -494,6 +519,47 @@ function spawnJsWorker(): WorkerHandle { } } +function spawnJsProcess(): WorkerHandle { + const spawned = createWorkerSubprocess({ + spawnCommand: resolveWorkerSpawnCmd(JS_EVAL_PROCESS_ARG), + env: workerEnvFromParent(), + exitLabel: "JS eval worker", + detached: shouldDetachKernel(process.platform), + reportCleanExit: true, + unref: false, + }); + const base = createWorkerHandle(spawned, message => + safeSendIpc(spawned.proc, message, "js-eval"), + ); + return { + mode: "process", + send: message => base.send(message), + onMessage: handler => base.onMessage(handler), + onError: handler => base.onError(handler), + async close() { + const { promise, resolve } = Promise.withResolvers(); + let settled = false; + let timeout: NodeJS.Timeout | undefined; + let unsubscribe = (): void => {}; + const finish = (value: boolean): void => { + if (settled) return; + settled = true; + if (timeout) clearTimeout(timeout); + unsubscribe(); + resolve(value); + }; + unsubscribe = base.onMessage(message => { + if (message.type !== "closed") return; + void base.terminate().finally(() => finish(true)); + }); + timeout = setTimeout(() => finish(false), workerCloseTimeoutMs); + base.send({ type: "close" }); + return await promise; + }, + terminate: () => base.terminate(), + }; +} + function wrapBunWorker(worker: Worker): WorkerHandle { return { mode: "worker", diff --git a/packages/coding-agent/src/eval/js/process-entry.ts b/packages/coding-agent/src/eval/js/process-entry.ts new file mode 100644 index 000000000..35644391f --- /dev/null +++ b/packages/coding-agent/src/eval/js/process-entry.ts @@ -0,0 +1,16 @@ +import { WorkerCore } from "./worker-core"; +import type { WorkerInbound, WorkerOutbound } from "./worker-protocol"; + +/** Start the JavaScript evaluator inside a subprocess IPC transport. */ +export function startJsEvalProcess(transport: { + send(message: WorkerOutbound): void; + onMessage(handler: (message: WorkerInbound) => void): () => void; +}): void { + new WorkerCore({ + send: message => transport.send(message), + onMessage: handler => transport.onMessage(handler), + // The parent owns process lifetime and kills the subprocess after the + // WorkerCore `closed` acknowledgement has crossed IPC. + close: () => {}, + }); +} diff --git a/packages/coding-agent/src/eval/py/kernel.ts b/packages/coding-agent/src/eval/py/kernel.ts index c69bb8db2..945db5561 100644 --- a/packages/coding-agent/src/eval/py/kernel.ts +++ b/packages/coding-agent/src/eval/py/kernel.ts @@ -23,7 +23,7 @@ import { resolveExplicitPythonRuntime, resolvePythonRuntime, } from "./runtime"; -import { hostHasInheritableConsole, shouldHideKernelWindow } from "./spawn-options"; +import { hostHasInheritableConsole, shouldDetachKernel, shouldHideKernelWindow } from "./spawn-options"; export type { KernelExecuteOptions, @@ -193,6 +193,7 @@ export class PythonKernel extends BaseKernel { const proc = Bun.spawn([runtime.pythonPath, "-u", scriptPath], { cwd: options.cwd, + detached: shouldDetachKernel(process.platform), env: spawnEnv, stdin: "pipe", stdout: "pipe", diff --git a/packages/coding-agent/src/eval/py/spawn-options.ts b/packages/coding-agent/src/eval/py/spawn-options.ts index c422577f9..2d244ff26 100644 --- a/packages/coding-agent/src/eval/py/spawn-options.ts +++ b/packages/coding-agent/src/eval/py/spawn-options.ts @@ -40,6 +40,19 @@ export function shouldHideKernelWindow(opts: { return !opts.hostHasInheritableConsole; } +/** + * Keep eval kernels outside the host's POSIX terminal session. + * + * User code can start an interactive shell which calls `tcsetpgrp(3)`. If the + * kernel shares OMP's session, that shell can replace OMP as the controlling + * terminal's foreground process group and the host is then stopped by SIGTTIN + * on its next stdin read. Bun implements `detached: true` with `setsid(2)` on + * POSIX, making the kernel a session leader with no controlling terminal. + */ +export function shouldDetachKernel(platform: NodeJS.Platform): boolean { + return platform !== "win32"; +} + /** * TTY-based fallback used when the Win32 console probe is unavailable. * diff --git a/packages/coding-agent/src/eval/rb/kernel.ts b/packages/coding-agent/src/eval/rb/kernel.ts index 192c0d049..eba3c281a 100644 --- a/packages/coding-agent/src/eval/rb/kernel.ts +++ b/packages/coding-agent/src/eval/rb/kernel.ts @@ -16,7 +16,7 @@ import { $ } from "bun"; import { Settings } from "../../config/settings"; import { BaseKernel, getRemainingTimeMs, type KernelRuntimeEnv, type KernelStartOptions } from "../kernel-base"; import type { KernelDisplayOutput } from "../py/display"; -import { hostHasInheritableConsole, shouldHideKernelWindow } from "../py/spawn-options"; +import { hostHasInheritableConsole, shouldDetachKernel, shouldHideKernelWindow } from "../py/spawn-options"; import { RUBY_PRELUDE } from "./prelude"; import RUNNER_SCRIPT from "./runner.rb" with { type: "text" }; import { @@ -186,6 +186,7 @@ export class RubyKernel extends BaseKernel { const proc = Bun.spawn([runtime.rubyPath, scriptPath], { cwd: options.cwd, + detached: shouldDetachKernel(process.platform), env: spawnEnv, stdin: "pipe", stdout: "pipe", diff --git a/packages/coding-agent/src/subprocess/worker-client.ts b/packages/coding-agent/src/subprocess/worker-client.ts index 59c7bbf6a..f9fdf5033 100644 --- a/packages/coding-agent/src/subprocess/worker-client.ts +++ b/packages/coding-agent/src/subprocess/worker-client.ts @@ -159,6 +159,12 @@ export function createWorkerSubprocess(options: { spawnCommand: WorkerSpawnCommand; env: Record; exitLabel: string; + /** Start the child as a new process-group/session leader where Bun supports it. */ + detached?: boolean; + /** Treat exit code 0 as unexpected; eval cells can call process.exit(0). */ + reportCleanExit?: boolean; + /** Whether an idle worker should stop keeping the parent event loop alive. */ + unref?: boolean; }): SpawnedSubprocess { const inbound = new Set<(message: Outbound) => void>(); const errors = new Set<(error: Error) => void>(); @@ -175,6 +181,7 @@ export function createWorkerSubprocess(options: { const proc = Bun.spawn({ cmd: options.spawnCommand.cmd, cwd: options.spawnCommand.cwd, + detached: options.detached, env: options.env, stdin: "ignore", stdout: "ignore", @@ -186,7 +193,7 @@ export function createWorkerSubprocess(options: { }, onExit(_proc, exitCode, signalCode) { startStderrDrain(); - if (exitCode === 0) return; + if (exitCode === 0 && !options.reportCleanExit) return; // Swallow only the expected SIGKILL from `terminate()`; every other // signal exit (SIGSEGV from a native fault, OOM SIGKILL, operator // `kill -9`) is a real worker death that must fault in-flight @@ -206,7 +213,7 @@ export function createWorkerSubprocess(options: { // Don't keep the parent event loop alive on an idle worker; the dispose // path calls `terminate()` explicitly. Bun's test runner starves IPC for // unref'd subprocesses, so keep it referenced only under tests. - if (!isBunTestRuntime()) proc.unref(); + if (!isBunTestRuntime() && options.unref !== false) proc.unref(); return { proc, inbound, errors, intentionalExit, stderrDrained: stderrDrained.promise }; } diff --git a/packages/coding-agent/test/core/python-runner.integration.test.ts b/packages/coding-agent/test/core/python-runner.integration.test.ts index 182379494..a564d8042 100644 --- a/packages/coding-agent/test/core/python-runner.integration.test.ts +++ b/packages/coding-agent/test/core/python-runner.integration.test.ts @@ -67,6 +67,18 @@ describe.skipIf(!SHOULD_RUN)("python runner subprocess", () => { } }); + it.skipIf(process.platform === "win32")("runs in its own POSIX session", async () => { + using tempDir = TempDir.createSync("@python-runner-session-isolation-"); + const kernel = await PythonKernel.start({ cwd: tempDir.path() }); + try { + const result = await executePythonWithKernel(kernel, "import os; print(os.getsid(0), os.getpid())"); + const [sessionId, processId] = result.output.trim().split(/\s+/).map(Number); + expect(sessionId).toBe(processId); + } finally { + await kernel.shutdown(); + } + }); + it("cancels a long sleep via SIGINT within 500ms", async () => { using tempDir = TempDir.createSync("@python-runner-cancel-"); const kernel = await PythonKernel.start({ cwd: tempDir.path() }); diff --git a/packages/coding-agent/test/core/ruby-runner.integration.test.ts b/packages/coding-agent/test/core/ruby-runner.integration.test.ts index c7ad56a76..56d4538f0 100644 --- a/packages/coding-agent/test/core/ruby-runner.integration.test.ts +++ b/packages/coding-agent/test/core/ruby-runner.integration.test.ts @@ -34,6 +34,18 @@ describe.skipIf(!SHOULD_RUN)("ruby runner subprocess", () => { } }); + it.skipIf(process.platform === "win32")("runs in its own POSIX session", async () => { + using tempDir = TempDir.createSync("@ruby-runner-session-isolation-"); + const kernel = await RubyKernel.start({ cwd: tempDir.path() }); + try { + const result = await executeRubyWithKernel(kernel, 'puts "#{Process.getsid(0)} #{Process.pid}"', {}); + const [sessionId, processId] = result.output.trim().split(/\s+/).map(Number); + expect(sessionId).toBe(processId); + } finally { + await kernel.shutdown(); + } + }); + it("keeps local variables across cells on one kernel", async () => { using tempDir = TempDir.createSync("@ruby-runner-state-"); const kernel = await RubyKernel.start({ cwd: tempDir.path() });