From cb1cef6747f6e6422ebe99da59a7418d44a6d951 Mon Sep 17 00:00:00 2001 From: Hona <10430890+Hona@users.noreply.github.com> Date: Tue, 21 Jul 2026 00:40:22 +0000 Subject: [PATCH] fix(opencode): bound shell output after exit --- packages/core/src/cross-spawn-spawner.ts | 5 +-- .../test/effect/cross-spawn-spawner.test.ts | 39 ++++++++++++++++ packages/opencode/src/tool/shell.ts | 17 ++++++- packages/opencode/test/tool/shell.test.ts | 45 +++++++++++++++++++ 4 files changed, 101 insertions(+), 5 deletions(-) diff --git a/packages/core/src/cross-spawn-spawner.ts b/packages/core/src/cross-spawn-spawner.ts index 6ea9022acf..82bb3e8a97 100644 --- a/packages/core/src/cross-spawn-spawner.ts +++ b/packages/core/src/cross-spawn-spawner.ts @@ -268,17 +268,16 @@ export const make = Effect.gen(function* () { Effect.callback((resume) => { const signal = Deferred.makeUnsafe() const proc = launch(command.command, command.args, opts) - let end = false let exit: readonly [code: number | null, signal: NodeJS.Signals | null] | undefined proc.on("error", (err) => { resume(Effect.fail(toPlatformError("spawn", err, command))) }) proc.on("exit", (...args) => { exit = args + Deferred.doneUnsafe(signal, Exit.succeed(args)) }) proc.on("close", (...args) => { - if (end) return - end = true + // cross-spawn can suppress `exit` for a Windows ENOENT and only emit `close`. Deferred.doneUnsafe(signal, Exit.succeed(exit ?? args)) }) proc.on("spawn", () => { diff --git a/packages/core/test/effect/cross-spawn-spawner.test.ts b/packages/core/test/effect/cross-spawn-spawner.test.ts index fc4e3db6a9..b81f49e943 100644 --- a/packages/core/test/effect/cross-spawn-spawner.test.ts +++ b/packages/core/test/effect/cross-spawn-spawner.test.ts @@ -285,6 +285,45 @@ describe("cross-spawn spawner", () => { expect(running).toBe(false) }), ) + + fx.effect( + "resolves exit before inherited output pipes close", + Effect.gen(function* () { + const tmp = yield* Effect.acquireRelease( + Effect.promise(() => tmpdir()), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ) + const file = path.join(tmp.path, "child.pid") + const child = "setInterval(() => {}, 10_000)" + const handle = yield* ChildProcess.make(process.execPath, [ + "-e", + [ + 'const { spawn } = require("node:child_process")', + 'const { writeFileSync } = require("node:fs")', + `const child = spawn(process.execPath, ["-e", ${JSON.stringify(child)}], { detached: true, stdio: "inherit" })`, + "child.unref()", + `writeFileSync(${JSON.stringify(file)}, String(child.pid))`, + ].join("\n"), + ]) + const code = yield* handle.exitCode.pipe( + Effect.timeoutOrElse({ + duration: "2 seconds", + orElse: () => Effect.die("exitCode waited for inherited output pipes"), + }), + ) + const pid = Number(yield* Effect.promise(() => fs.readFile(file, "utf8"))) + yield* Effect.addFinalizer(() => + Effect.sync(() => { + try { + process.kill(pid, "SIGKILL") + } catch {} + }), + ) + + expect(code).toBe(ChildProcessSpawner.ExitCode(0)) + expect(yield* handle.isRunning).toBe(false) + }), + ) }) describe("error handling", () => { diff --git a/packages/opencode/src/tool/shell.ts b/packages/opencode/src/tool/shell.ts index 1e4423e017..d25ef16976 100644 --- a/packages/opencode/src/tool/shell.ts +++ b/packages/opencode/src/tool/shell.ts @@ -1,4 +1,4 @@ -import { Effect, Stream } from "effect" +import { Effect, Fiber, Stream } from "effect" import os from "os" import { createWriteStream } from "node:fs" import * as Tool from "./tool" @@ -25,6 +25,7 @@ import { BashArity } from "@/permission/arity" export { Parameters } from "./shell/prompt" const MAX_METADATA_LENGTH = 30_000 +const POST_EXIT_OUTPUT_GRACE = "500 millis" const CWD = new Set(["cd", "chdir", "popd", "pushd", "push-location", "set-location"]) const FILES = new Set([ ...CWD, @@ -446,6 +447,7 @@ export const ShellTool = Tool.define( let cut = false let expired = false let aborted = false + let incomplete = false const closeSink = Effect.fnUntraced(function* () { const stream = sink @@ -483,7 +485,7 @@ export const ShellTool = Tool.define( yield* Effect.addFinalizer(closeSink) const handle = yield* spawner.spawn(cmd(input.shell, input.command, input.cwd, input.env)) - yield* Effect.forkScoped( + const output = yield* Effect.forkScoped( Stream.runForEach(Stream.decodeText(handle.all), (chunk) => { const size = Buffer.byteLength(chunk, "utf-8") list.push({ text: chunk, size }) @@ -554,6 +556,15 @@ export const ShellTool = Tool.define( yield* handle.kill({ forceKillAfter: "3 seconds" }).pipe(Effect.orDie) } + const outputComplete = yield* Fiber.await(output).pipe( + Effect.as(true), + Effect.timeoutOrElse({ + duration: POST_EXIT_OUTPUT_GRACE, + orElse: () => Effect.succeed(false), + }), + ) + if (!outputComplete) incomplete = true + return exit.kind === "exit" ? exit.code : null }), ).pipe(Effect.orDie) @@ -565,6 +576,7 @@ export const ShellTool = Tool.define( ) } if (aborted) meta.push("User aborted the command") + if (incomplete) meta.push("shell tool exited without reaching EOF within 500 ms") const raw = list.map((item) => item.text).join("") const end = tail(raw, limits.maxLines, limits.maxBytes) if (end.cut) cut = true @@ -588,6 +600,7 @@ export const ShellTool = Tool.define( output: last || preview(output), exit: code, truncated: cut, + ...(incomplete ? { outputIncomplete: true } : {}), ...(cut && file ? { outputPath: file } : {}), }, output, diff --git a/packages/opencode/test/tool/shell.test.ts b/packages/opencode/test/tool/shell.test.ts index a970f85d46..81c2da7678 100644 --- a/packages/opencode/test/tool/shell.test.ts +++ b/packages/opencode/test/tool/shell.test.ts @@ -152,6 +152,15 @@ const withShell = (item: { label: string; shell: string }, self: Effect }), ) +const kill = (file: string) => + Effect.promise(async () => { + const pid = Number(await Bun.file(file).text().catch(() => "")) + if (!pid) return + try { + process.kill(pid, "SIGKILL") + } catch {} + }) + const each = ( name: string, fn: (item: { label: string; shell: string }) => Effect.Effect, @@ -1077,6 +1086,42 @@ describe("tool.shell abort", () => { 15_000, ) + it.live( + "stops waiting when a descendant retains the output pipe", + () => + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + const file = path.join(tmp, "parent.js") + const pid = path.join(tmp, "child.pid") + const child = 'setInterval(() => process.stdout.write("tick\\n"), 20)' + yield* Effect.promise(() => + Bun.write( + file, + [ + 'const { spawn } = require("node:child_process")', + 'const { writeFileSync } = require("node:fs")', + `const child = spawn(process.execPath, ["-e", ${JSON.stringify(child)}], { detached: true, stdio: "inherit" })`, + "child.unref()", + `writeFileSync(${JSON.stringify(pid)}, String(child.pid))`, + 'console.log("parent done")', + ].join("\n"), + ), + ) + const command = `${bin} ${quote(file.replaceAll("\\", "/"))}` + const started = Date.now() + const result = yield* runIn( + tmp, + run({ command: PS.has(sh()) ? `& ${command}` : command, timeout: 5_000 }), + ).pipe(Effect.ensuring(kill(pid))) + + expect(Date.now() - started).toBeLessThan(3_000) + expect(result.output).toContain("parent done") + expect(result.output).toContain("shell tool exited without reaching EOF within 500 ms") + expect(result.metadata.outputIncomplete).toBe(true) + }), + 15_000, + ) + if (process.platform !== "win32") { it.live("captures stderr in output", () => runIn(