From a9c85b7c2789f9363cbfeb9c1adceaddfbdbbdc3 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Wed, 1 Apr 2026 12:07:57 -0400 Subject: [PATCH] refactor(shell): use Effect ChildProcess for shell command execution (#20494) --- packages/app/e2e/prompt/prompt-shell.spec.ts | 6 +- .../src/effect/cross-spawn-spawner.ts | 33 ++++--- packages/opencode/src/session/prompt.ts | 95 ++++++++----------- .../test/session/prompt-effect.test.ts | 74 +++++++++++++++ 4 files changed, 137 insertions(+), 71 deletions(-) diff --git a/packages/app/e2e/prompt/prompt-shell.spec.ts b/packages/app/e2e/prompt/prompt-shell.spec.ts index 4c92f4a2f2..019219bf5e 100644 --- a/packages/app/e2e/prompt/prompt-shell.spec.ts +++ b/packages/app/e2e/prompt/prompt-shell.spec.ts @@ -2,7 +2,6 @@ import type { ToolPart } from "@opencode-ai/sdk/v2/client" import { test, expect } from "../fixtures" import { sessionIDFromUrl } from "../actions" import { promptSelector } from "../selectors" -import { createSdk } from "../utils" const isBash = (part: unknown): part is ToolPart => { if (!part || typeof part !== "object") return false @@ -14,10 +13,9 @@ const isBash = (part: unknown): part is ToolPart => { test("shell mode runs a command in the project directory", async ({ page, withProject }) => { test.setTimeout(120_000) - await withProject(async ({ directory, gotoSession, trackSession }) => { - const sdk = createSdk(directory) + await withProject(async ({ directory, gotoSession, trackSession, sdk }) => { const prompt = page.locator(promptSelector) - const cmd = process.platform === "win32" ? "dir" : "ls" + const cmd = process.platform === "win32" ? "dir" : "command ls" await gotoSession() await prompt.click() diff --git a/packages/opencode/src/effect/cross-spawn-spawner.ts b/packages/opencode/src/effect/cross-spawn-spawner.ts index fc08805744..14b717611b 100644 --- a/packages/opencode/src/effect/cross-spawn-spawner.ts +++ b/packages/opencode/src/effect/cross-spawn-spawner.ts @@ -386,9 +386,17 @@ export const make = Effect.gen(function* () { if (code !== 0 && Predicate.isNotNull(code)) return yield* Effect.ignore(kill(killGroup)) return yield* Effect.void } - return yield* kill((command, proc, signal) => - Effect.catch(killGroup(command, proc, signal), () => killOne(command, proc, signal)), - ).pipe(Effect.andThen(Deferred.await(signal)), Effect.ignore) + const send = (s: NodeJS.Signals) => + Effect.catch(killGroup(command, proc, s), () => killOne(command, proc, s)) + const sig = command.options.killSignal ?? "SIGTERM" + const attempt = send(sig).pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid) + const escalated = command.options.forceKillAfter + ? Effect.timeoutOrElse(attempt, { + duration: command.options.forceKillAfter, + orElse: () => send("SIGKILL").pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid), + }) + : attempt + return yield* Effect.ignore(escalated) }), ) @@ -413,14 +421,17 @@ export const make = Effect.gen(function* () { ), ) }), - kill: (opts?: ChildProcess.KillOptions) => - timeout( - proc, - command, - opts, - )((command, proc, signal) => - Effect.catch(killGroup(command, proc, signal), () => killOne(command, proc, signal)), - ).pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid), + kill: (opts?: ChildProcess.KillOptions) => { + const sig = opts?.killSignal ?? "SIGTERM" + const send = (s: NodeJS.Signals) => + Effect.catch(killGroup(command, proc, s), () => killOne(command, proc, s)) + const attempt = send(sig).pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid) + if (!opts?.forceKillAfter) return attempt + return Effect.timeoutOrElse(attempt, { + duration: opts.forceKillAfter, + orElse: () => send("SIGKILL").pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid), + }) + }, }) } case "PipedCommand": { diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 821069cca3..fbce62e7dc 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -28,7 +28,9 @@ import { ReadTool } from "../tool/read" import { FileTime } from "../file/time" import { Flag } from "../flag/flag" import { ulid } from "ulid" -import { spawn } from "child_process" +import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process" +import * as CrossSpawnSpawner from "@/effect/cross-spawn-spawner" +import * as Stream from "effect/Stream" import { Command } from "../command" import { pathToFileURL, fileURLToPath } from "url" import { ConfigMarkdown } from "../config/markdown" @@ -96,6 +98,7 @@ export namespace SessionPrompt { const filetime = yield* FileTime.Service const registry = yield* ToolRegistry.Service const truncate = yield* Truncate.Service + const spawner = yield* ChildProcessSpawner.ChildProcessSpawner const scope = yield* Scope.Scope const state = yield* InstanceState.make( @@ -809,22 +812,26 @@ NOTE: At any point in time through this workflow you should feel free to ask the fish: { args: ["-c", input.command] }, zsh: { args: [ - "-c", "-l", + "-c", ` + __oc_cwd=$PWD [[ -f ~/.zshenv ]] && source ~/.zshenv >/dev/null 2>&1 || true [[ -f "\${ZDOTDIR:-$HOME}/.zshrc" ]] && source "\${ZDOTDIR:-$HOME}/.zshrc" >/dev/null 2>&1 || true + cd "$__oc_cwd" eval ${JSON.stringify(input.command)} `, ], }, bash: { args: [ - "-c", "-l", + "-c", ` + __oc_cwd=$PWD shopt -s expand_aliases [[ -f ~/.bashrc ]] && source ~/.bashrc >/dev/null 2>&1 || true + cd "$__oc_cwd" eval ${JSON.stringify(input.command)} `, ], @@ -832,7 +839,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the cmd: { args: ["/c", input.command] }, powershell: { args: ["-NoProfile", "-Command", input.command] }, pwsh: { args: ["-NoProfile", "-Command", input.command] }, - "": { args: ["-c", `${input.command}`] }, + "": { args: ["-c", input.command] }, } const args = (invocations[shellName] ?? invocations[""]).args @@ -842,51 +849,20 @@ NOTE: At any point in time through this workflow you should feel free to ask the { cwd, sessionID: input.sessionID, callID: part.callID }, { env: {} }, ) - const proc = yield* Effect.sync(() => - spawn(sh, args, { - cwd, - detached: process.platform !== "win32", - windowsHide: process.platform === "win32", - stdio: ["ignore", "pipe", "pipe"], - env: { - ...process.env, - ...shellEnv.env, - TERM: "dumb", - }, - }), - ) + + const cmd = ChildProcess.make(sh, args, { + cwd, + extendEnv: true, + env: { ...shellEnv.env, TERM: "dumb" }, + stdin: "ignore", + forceKillAfter: "3 seconds", + }) let output = "" - const write = () => { - if (part.state.status !== "running") return - part.state.metadata = { output, description: "" } - void Effect.runFork(sessions.updatePart(part)) - } - - proc.stdout?.on("data", (chunk) => { - output += chunk.toString() - write() - }) - proc.stderr?.on("data", (chunk) => { - output += chunk.toString() - write() - }) - let aborted = false - let exited = false - let finished = false - const kill = Effect.promise(() => Shell.killTree(proc, { exited: () => exited })) - - const abortHandler = () => { - if (aborted) return - aborted = true - void Effect.runFork(kill) - } const finish = Effect.uninterruptible( Effect.gen(function* () { - if (finished) return - finished = true if (aborted) { output += "\n\n" + ["", "User aborted the command", ""].join("\n") } @@ -908,20 +884,26 @@ NOTE: At any point in time through this workflow you should feel free to ask the }), ) - const exit = yield* Effect.promise(() => { - signal.addEventListener("abort", abortHandler, { once: true }) - if (signal.aborted) abortHandler() - return new Promise((resolve) => { - const close = () => { - exited = true - proc.off("close", close) - resolve() - } - proc.once("close", close) - }) + const exit = yield* Effect.gen(function* () { + const handle = yield* spawner.spawn(cmd) + yield* Stream.runForEach(Stream.decodeText(handle.all), (chunk) => + Effect.sync(() => { + output += chunk + if (part.state.status === "running") { + part.state.metadata = { output, description: "" } + void Effect.runFork(sessions.updatePart(part)) + } + }), + ) + yield* handle.exitCode }).pipe( - Effect.onInterrupt(() => Effect.sync(abortHandler)), - Effect.ensuring(Effect.sync(() => signal.removeEventListener("abort", abortHandler))), + Effect.scoped, + Effect.onInterrupt(() => + Effect.sync(() => { + aborted = true + }), + ), + Effect.orDie, Effect.ensuring(finish), Effect.exit, ) @@ -1735,6 +1717,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the Layer.provide(Session.defaultLayer), Layer.provide(Agent.defaultLayer), Layer.provide(Bus.layer), + Layer.provide(CrossSpawnSpawner.defaultLayer), ), ), ) diff --git a/packages/opencode/test/session/prompt-effect.test.ts b/packages/opencode/test/session/prompt-effect.test.ts index 28b4cf15cb..0a6c8e02c8 100644 --- a/packages/opencode/test/session/prompt-effect.test.ts +++ b/packages/opencode/test/session/prompt-effect.test.ts @@ -1,6 +1,7 @@ import { NodeFileSystem } from "@effect/platform-node" import { expect, spyOn } from "bun:test" import { Cause, Effect, Exit, Fiber, Layer } from "effect" +import path from "path" import z from "zod" import type { Agent } from "../../src/agent/agent" import { Agent as AgentSvc } from "../../src/agent/agent" @@ -887,6 +888,79 @@ unix("shell captures stdout and stderr in completed tool output", () => ), ) +unix("shell completes a fast command on the preferred shell", () => + provideTmpdirInstance( + (dir) => + Effect.gen(function* () { + const { prompt, chat } = yield* boot() + const result = yield* prompt.shell({ + sessionID: chat.id, + agent: "build", + command: "pwd", + }) + + expect(result.info.role).toBe("assistant") + const tool = completedTool(result.parts) + if (!tool) return + + expect(tool.state.input.command).toBe("pwd") + expect(tool.state.output).toContain(dir) + expect(tool.state.metadata.output).toContain(dir) + yield* prompt.assertNotBusy(chat.id) + }), + { git: true, config: cfg }, + ), +) + +unix("shell lists files from the project directory", () => + provideTmpdirInstance( + (dir) => + Effect.gen(function* () { + const { prompt, chat } = yield* boot() + yield* Effect.promise(() => Bun.write(path.join(dir, "README.md"), "# e2e\n")) + + const result = yield* prompt.shell({ + sessionID: chat.id, + agent: "build", + command: "command ls", + }) + + expect(result.info.role).toBe("assistant") + const tool = completedTool(result.parts) + if (!tool) return + + expect(tool.state.input.command).toBe("command ls") + expect(tool.state.output).toContain("README.md") + expect(tool.state.metadata.output).toContain("README.md") + yield* prompt.assertNotBusy(chat.id) + }), + { git: true, config: cfg }, + ), +) + +unix("shell captures stderr from a failing command", () => + provideTmpdirInstance( + (dir) => + Effect.gen(function* () { + const { prompt, chat } = yield* boot() + const result = yield* prompt.shell({ + sessionID: chat.id, + agent: "build", + command: "command -v __nonexistent_cmd_e2e__ || echo 'not found' >&2; exit 1", + }) + + expect(result.info.role).toBe("assistant") + const tool = completedTool(result.parts) + if (!tool) return + + expect(tool.state.output).toContain("not found") + expect(tool.state.metadata.output).toContain("not found") + yield* prompt.assertNotBusy(chat.id) + }), + { git: true, config: cfg }, + ), +) + unix( "shell updates running metadata before process exit", () =>