refactor(shell): use Effect ChildProcess for shell command execution (#20494)

Author: kitlangton

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()

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": {

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,30 +812,34 @@ 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)}
               `,
             ],
           },
           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",
-            },
-          }),
-        )
 
-        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()
+        const cmd = ChildProcess.make(sh, args, {
+          cwd,
+          extendEnv: true,
+          env: { ...shellEnv.env, TERM: "dumb" },
+          stdin: "ignore",
+          forceKillAfter: "3 seconds",
         })
 
+        let output = ""
         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" + ["<metadata>", "User aborted the command", "</metadata>"].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<void>((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),
       ),
     ),
   )

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",
   () =>