From e4ff1ea7784692d328b8f1510776648c34d87d9b Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Wed, 1 Apr 2026 19:48:47 -0400 Subject: [PATCH] refactor(bash): use Effect ChildProcess for bash tool execution (#20496) --- .../src/effect/cross-spawn-spawner.ts | 11 ++ packages/opencode/src/tool/bash.ts | 146 +++++++++--------- packages/opencode/test/tool/bash.test.ts | 115 ++++++++++++++ 3 files changed, 201 insertions(+), 71 deletions(-) diff --git a/packages/opencode/src/effect/cross-spawn-spawner.ts b/packages/opencode/src/effect/cross-spawn-spawner.ts index 14b717611b..30b4dde428 100644 --- a/packages/opencode/src/effect/cross-spawn-spawner.ts +++ b/packages/opencode/src/effect/cross-spawn-spawner.ts @@ -488,3 +488,14 @@ export const layer: Layer.Layer { + // Dynamic import to avoid circular dep: cross-spawn-spawner → run-service → Instance → project → cross-spawn-spawner + const { makeRuntime } = require("@/effect/run-service") as typeof import("@/effect/run-service") + return makeRuntime(ChildProcessSpawner, defaultLayer) +}) + +export const runPromiseExit: ReturnType["runPromiseExit"] = (...args) => rt().runPromiseExit(...(args as [any])) +export const runPromise: ReturnType["runPromise"] = (...args) => rt().runPromise(...(args as [any])) diff --git a/packages/opencode/src/tool/bash.ts b/packages/opencode/src/tool/bash.ts index 50aa9e14ad..119cbadfa1 100644 --- a/packages/opencode/src/tool/bash.ts +++ b/packages/opencode/src/tool/bash.ts @@ -1,6 +1,5 @@ import z from "zod" import os from "os" -import { spawn } from "child_process" import { Tool } from "./tool" import path from "path" import DESCRIPTION from "./bash.txt" @@ -18,6 +17,9 @@ import { Shell } from "@/shell/shell" import { BashArity } from "@/permission/arity" import { Truncate } from "./truncate" import { Plugin } from "@/plugin" +import { Cause, Effect, Exit, Stream } from "effect" +import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process" +import * as CrossSpawnSpawner from "@/effect/cross-spawn-spawner" const MAX_METADATA_LENGTH = 30_000 const DEFAULT_TIMEOUT = Flag.OPENCODE_EXPERIMENTAL_BASH_DEFAULT_TIMEOUT_MS || 2 * 60 * 1000 @@ -293,27 +295,26 @@ async function shellEnv(ctx: Tool.Context, cwd: string) { } } -function launch(shell: string, name: string, command: string, cwd: string, env: NodeJS.ProcessEnv) { +function cmd(shell: string, name: string, command: string, cwd: string, env: NodeJS.ProcessEnv) { if (process.platform === "win32" && PS.has(name)) { - return spawn(shell, ["-NoLogo", "-NoProfile", "-NonInteractive", "-Command", command], { + return ChildProcess.make(shell, ["-NoLogo", "-NoProfile", "-NonInteractive", "-Command", command], { cwd, env, - stdio: ["ignore", "pipe", "pipe"], + stdin: "ignore", detached: false, - windowsHide: true, }) } - return spawn(command, { + return ChildProcess.make(command, [], { shell, cwd, env, - stdio: ["ignore", "pipe", "pipe"], + stdin: "ignore", detached: process.platform !== "win32", - windowsHide: process.platform === "win32", }) } + async function run( input: { shell: string @@ -326,8 +327,9 @@ async function run( }, ctx: Tool.Context, ) { - const proc = launch(input.shell, input.name, input.command, input.cwd, input.env) let output = "" + let expired = false + let aborted = false ctx.metadata({ metadata: { @@ -336,76 +338,78 @@ async function run( }, }) - const append = (chunk: Buffer) => { - output += chunk.toString() - ctx.metadata({ - metadata: { - output: preview(output), - description: input.description, - }, - }) + const exit = await CrossSpawnSpawner.runPromiseExit((spawner) => + Effect.gen(function* () { + const handle = yield* spawner.spawn( + cmd(input.shell, input.name, input.command, input.cwd, input.env), + ) + + yield* Effect.forkScoped( + Stream.runForEach( + Stream.decodeText(handle.all), + (chunk) => + Effect.sync(() => { + output += chunk + ctx.metadata({ + metadata: { + output: preview(output), + description: input.description, + }, + }) + }), + ), + ) + + const abort = Effect.callback((resume) => { + if (ctx.abort.aborted) return resume(Effect.void) + const handler = () => resume(Effect.void) + ctx.abort.addEventListener("abort", handler, { once: true }) + return Effect.sync(() => ctx.abort.removeEventListener("abort", handler)) + }) + + const timeout = Effect.sleep(`${input.timeout + 100} millis`) + + const exit = yield* Effect.raceAll([ + handle.exitCode.pipe(Effect.map((code) => ({ kind: "exit" as const, code }))), + abort.pipe(Effect.map(() => ({ kind: "abort" as const, code: null }))), + timeout.pipe(Effect.map(() => ({ kind: "timeout" as const, code: null }))), + ]) + + if (exit.kind === "abort") { + aborted = true + yield* handle.kill({ forceKillAfter: "3 seconds" }).pipe(Effect.orDie) + } + if (exit.kind === "timeout") { + expired = true + yield* handle.kill({ forceKillAfter: "3 seconds" }).pipe(Effect.orDie) + } + + return exit.kind === "exit" ? exit.code : null + }).pipe( + Effect.scoped, + Effect.orDie, + ), + ) + + let code: number | null = null + if (Exit.isSuccess(exit)) { + code = exit.value + } else if (!Cause.hasInterruptsOnly(exit.cause)) { + throw Cause.squash(exit.cause) } - proc.stdout?.on("data", append) - proc.stderr?.on("data", append) - - let expired = false - let aborted = false - let exited = false - - const kill = () => Shell.killTree(proc, { exited: () => exited }) - - if (ctx.abort.aborted) { - aborted = true - await kill() - } - - const abort = () => { - aborted = true - void kill() - } - - ctx.abort.addEventListener("abort", abort, { once: true }) - const timer = setTimeout(() => { - expired = true - void kill() - }, input.timeout + 100) - - await new Promise((resolve, reject) => { - const cleanup = () => { - clearTimeout(timer) - ctx.abort.removeEventListener("abort", abort) - } - - proc.once("exit", () => { - exited = true - }) - - proc.once("close", () => { - exited = true - cleanup() - resolve() - }) - - proc.once("error", (error) => { - exited = true - cleanup() - reject(error) - }) - }) - - const metadata: string[] = [] - if (expired) metadata.push(`bash tool terminated command after exceeding timeout ${input.timeout} ms`) - if (aborted) metadata.push("User aborted the command") - if (metadata.length > 0) { - output += "\n\n\n" + metadata.join("\n") + "\n" + const meta: string[] = [] + if (expired) meta.push(`bash tool terminated command after exceeding timeout ${input.timeout} ms`) + if (aborted) meta.push("User aborted the command") + if (meta.length > 0) { + output += "\n\n\n" + meta.join("\n") + "\n" } return { title: input.description, metadata: { output: preview(output), - exit: proc.exitCode, + exit: code, description: input.description, }, output, diff --git a/packages/opencode/test/tool/bash.test.ts b/packages/opencode/test/tool/bash.test.ts index 0ea8ea073a..e4ba881fb1 100644 --- a/packages/opencode/test/tool/bash.test.ts +++ b/packages/opencode/test/tool/bash.test.ts @@ -896,6 +896,121 @@ describe("tool.bash permissions", () => { }) }) +describe("tool.bash abort", () => { + test("preserves output when aborted", async () => { + await Instance.provide({ + directory: projectRoot, + fn: async () => { + const bash = await BashTool.init() + const controller = new AbortController() + const collected: string[] = [] + const result = bash.execute( + { + command: `echo before && sleep 30`, + description: "Long running command", + }, + { + ...ctx, + abort: controller.signal, + metadata: (input) => { + const output = (input.metadata as { output?: string })?.output + if (output && output.includes("before") && !controller.signal.aborted) { + collected.push(output) + controller.abort() + } + }, + }, + ) + const res = await result + expect(res.output).toContain("before") + expect(res.output).toContain("User aborted the command") + expect(collected.length).toBeGreaterThan(0) + }, + }) + }, 15_000) + + test("terminates command on timeout", async () => { + await Instance.provide({ + directory: projectRoot, + fn: async () => { + const bash = await BashTool.init() + const result = await bash.execute( + { + command: `echo started && sleep 60`, + description: "Timeout test", + timeout: 500, + }, + ctx, + ) + expect(result.output).toContain("started") + expect(result.output).toContain("bash tool terminated command after exceeding timeout") + }, + }) + }, 15_000) + + test.skipIf(process.platform === "win32")("captures stderr in output", async () => { + await Instance.provide({ + directory: projectRoot, + fn: async () => { + const bash = await BashTool.init() + const result = await bash.execute( + { + command: `echo stdout_msg && echo stderr_msg >&2`, + description: "Stderr test", + }, + ctx, + ) + expect(result.output).toContain("stdout_msg") + expect(result.output).toContain("stderr_msg") + expect(result.metadata.exit).toBe(0) + }, + }) + }) + + test("returns non-zero exit code", async () => { + await Instance.provide({ + directory: projectRoot, + fn: async () => { + const bash = await BashTool.init() + const result = await bash.execute( + { + command: `exit 42`, + description: "Non-zero exit", + }, + ctx, + ) + expect(result.metadata.exit).toBe(42) + }, + }) + }) + + test("streams metadata updates progressively", async () => { + await Instance.provide({ + directory: projectRoot, + fn: async () => { + const bash = await BashTool.init() + const updates: string[] = [] + const result = await bash.execute( + { + command: `echo first && sleep 0.1 && echo second`, + description: "Streaming test", + }, + { + ...ctx, + metadata: (input) => { + const output = (input.metadata as { output?: string })?.output + if (output) updates.push(output) + }, + }, + ) + expect(result.output).toContain("first") + expect(result.output).toContain("second") + expect(updates.length).toBeGreaterThan(1) + }, + }) + }) +}) + describe("tool.bash truncation", () => { test("truncates output exceeding line limit", async () => { await Instance.provide({