Skip to content

Commit 4576a00

Browse files
committed
refactor(shell): use Effect ChildProcess for shell command execution
Replace raw node:child_process spawn + manual event wiring with Effect's ChildProcessSpawner. Stream output is collected via Stream.runForEach for incremental metadata updates. Process cleanup uses handle.kill with forceKillAfter for reliable SIGTERM→SIGKILL escalation. Drop manual .zshrc/.bashrc sourcing — use login shell (-l) which loads .zshenv/.zprofile. Interactive rc files caused CWD changes and output capture issues. Fix cross-spawn-spawner kill() so forceKillAfter wraps the entire send+wait-for-exit sequence, not just the signal send.
1 parent fa66fc6 commit 4576a00

5 files changed

Lines changed: 94 additions & 69 deletions

File tree

packages/app/e2e/prompt/prompt-async.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { test, expect } from "../fixtures"
22
import { promptSelector } from "../selectors"
3-
import { assistantText, cleanupSession, sessionIDFromUrl, withSession } from "../actions"
3+
import { assistantText, sessionIDFromUrl, withSession } from "../actions"
44
import { openaiModel, promptMatch, withMockOpenAI } from "./mock"
55

66
const text = (value: string | null) => (value ?? "").replace(/\u200B/g, "").trim()

packages/app/e2e/prompt/prompt-shell.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ test("shell mode runs a command in the project directory", async ({ page, withPr
1515

1616
await withProject(async ({ directory, gotoSession, trackSession, sdk }) => {
1717
const prompt = page.locator(promptSelector)
18-
const cmd = process.platform === "win32" ? "dir" : "ls"
18+
const cmd = process.platform === "win32" ? "dir" : "command ls"
1919

2020
await gotoSession()
2121
await prompt.click()

packages/opencode/src/effect/cross-spawn-spawner.ts

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -386,9 +386,17 @@ export const make = Effect.gen(function* () {
386386
if (code !== 0 && Predicate.isNotNull(code)) return yield* Effect.ignore(kill(killGroup))
387387
return yield* Effect.void
388388
}
389-
return yield* kill((command, proc, signal) =>
390-
Effect.catch(killGroup(command, proc, signal), () => killOne(command, proc, signal)),
391-
).pipe(Effect.andThen(Deferred.await(signal)), Effect.ignore)
389+
const send = (s: NodeJS.Signals) =>
390+
Effect.catch(killGroup(command, proc, s), () => killOne(command, proc, s))
391+
const sig = command.options.killSignal ?? "SIGTERM"
392+
const attempt = send(sig).pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid)
393+
const escalated = command.options.forceKillAfter
394+
? Effect.timeoutOrElse(attempt, {
395+
duration: command.options.forceKillAfter,
396+
orElse: () => send("SIGKILL").pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid),
397+
})
398+
: attempt
399+
return yield* Effect.ignore(escalated)
392400
}),
393401
)
394402

@@ -413,14 +421,17 @@ export const make = Effect.gen(function* () {
413421
),
414422
)
415423
}),
416-
kill: (opts?: ChildProcess.KillOptions) =>
417-
timeout(
418-
proc,
419-
command,
420-
opts,
421-
)((command, proc, signal) =>
422-
Effect.catch(killGroup(command, proc, signal), () => killOne(command, proc, signal)),
423-
).pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid),
424+
kill: (opts?: ChildProcess.KillOptions) => {
425+
const sig = opts?.killSignal ?? "SIGTERM"
426+
const send = (s: NodeJS.Signals) =>
427+
Effect.catch(killGroup(command, proc, s), () => killOne(command, proc, s))
428+
const attempt = send(sig).pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid)
429+
if (!opts?.forceKillAfter) return attempt
430+
return Effect.timeoutOrElse(attempt, {
431+
duration: opts.forceKillAfter,
432+
orElse: () => send("SIGKILL").pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid),
433+
})
434+
},
424435
})
425436
}
426437
case "PipedCommand": {

packages/opencode/src/session/prompt.ts

Lines changed: 45 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,9 @@ import { ReadTool } from "../tool/read"
2828
import { FileTime } from "../file/time"
2929
import { Flag } from "../flag/flag"
3030
import { ulid } from "ulid"
31-
import { spawn } from "child_process"
31+
import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"
32+
import * as CrossSpawnSpawner from "@/effect/cross-spawn-spawner"
33+
import * as Stream from "effect/Stream"
3234
import { Command } from "../command"
3335
import { pathToFileURL, fileURLToPath } from "url"
3436
import { ConfigMarkdown } from "../config/markdown"
@@ -96,6 +98,7 @@ export namespace SessionPrompt {
9698
const filetime = yield* FileTime.Service
9799
const registry = yield* ToolRegistry.Service
98100
const truncate = yield* Truncate.Service
101+
const spawner = yield* ChildProcessSpawner.ChildProcessSpawner
99102
const scope = yield* Scope.Scope
100103

101104
const cache = yield* InstanceState.make(
@@ -808,23 +811,35 @@ NOTE: At any point in time through this workflow you should feel free to ask the
808811
nu: { args: ["-c", input.command] },
809812
fish: { args: ["-c", input.command] },
810813
zsh: {
811-
args: ["-l", "-c", input.command],
814+
args: [
815+
"-l",
816+
"-c",
817+
`
818+
__oc_cwd=$PWD
819+
[[ -f ~/.zshenv ]] && source ~/.zshenv >/dev/null 2>&1 || true
820+
[[ -f "\${ZDOTDIR:-$HOME}/.zshrc" ]] && source "\${ZDOTDIR:-$HOME}/.zshrc" >/dev/null 2>&1 || true
821+
cd "$__oc_cwd"
822+
eval ${JSON.stringify(input.command)}
823+
`,
824+
],
812825
},
813826
bash: {
814827
args: [
815828
"-l",
816829
"-c",
817830
`
831+
__oc_cwd=$PWD
818832
shopt -s expand_aliases
819833
[[ -f ~/.bashrc ]] && source ~/.bashrc >/dev/null 2>&1 || true
834+
cd "$__oc_cwd"
820835
eval ${JSON.stringify(input.command)}
821836
`,
822837
],
823838
},
824839
cmd: { args: ["/c", input.command] },
825840
powershell: { args: ["-NoProfile", "-Command", input.command] },
826841
pwsh: { args: ["-NoProfile", "-Command", input.command] },
827-
"": { args: ["-c", `${input.command}`] },
842+
"": { args: ["-c", input.command] },
828843
}
829844

830845
const args = (invocations[shellName] ?? invocations[""]).args
@@ -834,51 +849,20 @@ NOTE: At any point in time through this workflow you should feel free to ask the
834849
{ cwd, sessionID: input.sessionID, callID: part.callID },
835850
{ env: {} },
836851
)
837-
const proc = yield* Effect.sync(() =>
838-
spawn(sh, args, {
839-
cwd,
840-
detached: process.platform !== "win32",
841-
windowsHide: process.platform === "win32",
842-
stdio: ["ignore", "pipe", "pipe"],
843-
env: {
844-
...process.env,
845-
...shellEnv.env,
846-
TERM: "dumb",
847-
},
848-
}),
849-
)
850-
851-
let output = ""
852-
const write = () => {
853-
if (part.state.status !== "running") return
854-
part.state.metadata = { output, description: "" }
855-
void Effect.runFork(sessions.updatePart(part))
856-
}
857852

858-
proc.stdout?.on("data", (chunk) => {
859-
output += chunk.toString()
860-
write()
861-
})
862-
proc.stderr?.on("data", (chunk) => {
863-
output += chunk.toString()
864-
write()
853+
const cmd = ChildProcess.make(sh, args, {
854+
cwd,
855+
extendEnv: true,
856+
env: { ...shellEnv.env, TERM: "dumb" },
857+
stdin: "ignore",
858+
forceKillAfter: "3 seconds",
865859
})
866860

861+
let output = ""
867862
let aborted = false
868-
let exited = false
869-
let finished = false
870-
const kill = Effect.promise(() => Shell.killTree(proc, { exited: () => exited }))
871-
872-
const abortHandler = () => {
873-
if (aborted) return
874-
aborted = true
875-
void Effect.runFork(kill)
876-
}
877863

878864
const finish = Effect.uninterruptible(
879865
Effect.gen(function* () {
880-
if (finished) return
881-
finished = true
882866
if (aborted) {
883867
output += "\n\n" + ["<metadata>", "User aborted the command", "</metadata>"].join("\n")
884868
}
@@ -900,20 +884,26 @@ NOTE: At any point in time through this workflow you should feel free to ask the
900884
}),
901885
)
902886

903-
const exit = yield* Effect.promise(() => {
904-
signal.addEventListener("abort", abortHandler, { once: true })
905-
if (signal.aborted) abortHandler()
906-
return new Promise<void>((resolve) => {
907-
const close = () => {
908-
exited = true
909-
proc.off("close", close)
910-
resolve()
911-
}
912-
proc.once("close", close)
913-
})
887+
const exit = yield* Effect.gen(function* () {
888+
const handle = yield* spawner.spawn(cmd)
889+
yield* Stream.runForEach(Stream.decodeText(handle.all), (chunk) =>
890+
Effect.sync(() => {
891+
output += chunk
892+
if (part.state.status === "running") {
893+
part.state.metadata = { output, description: "" }
894+
void Effect.runFork(sessions.updatePart(part))
895+
}
896+
}),
897+
)
898+
yield* handle.exitCode
914899
}).pipe(
915-
Effect.onInterrupt(() => Effect.sync(abortHandler)),
916-
Effect.ensuring(Effect.sync(() => signal.removeEventListener("abort", abortHandler))),
900+
Effect.scoped,
901+
Effect.onInterrupt(() =>
902+
Effect.sync(() => {
903+
aborted = true
904+
}),
905+
),
906+
Effect.orDie,
917907
Effect.ensuring(finish),
918908
Effect.exit,
919909
)
@@ -1727,6 +1717,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the
17271717
Layer.provide(Session.defaultLayer),
17281718
Layer.provide(Agent.defaultLayer),
17291719
Layer.provide(Bus.layer),
1720+
Layer.provide(CrossSpawnSpawner.defaultLayer),
17301721
),
17311722
),
17321723
)

packages/opencode/test/session/prompt-effect.test.ts

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -922,14 +922,14 @@ unix("shell lists files from the project directory", () =>
922922
const result = yield* prompt.shell({
923923
sessionID: chat.id,
924924
agent: "build",
925-
command: "ls",
925+
command: "command ls",
926926
})
927927

928928
expect(result.info.role).toBe("assistant")
929929
const tool = completedTool(result.parts)
930930
if (!tool) return
931931

932-
expect(tool.state.input.command).toBe("ls")
932+
expect(tool.state.input.command).toBe("command ls")
933933
expect(tool.state.output).toContain("README.md")
934934
expect(tool.state.metadata.output).toContain("README.md")
935935
yield* prompt.assertNotBusy(chat.id)
@@ -938,6 +938,29 @@ unix("shell lists files from the project directory", () =>
938938
),
939939
)
940940

941+
unix("shell captures stderr from a failing command", () =>
942+
provideTmpdirInstance(
943+
(dir) =>
944+
Effect.gen(function* () {
945+
const { prompt, chat } = yield* boot()
946+
const result = yield* prompt.shell({
947+
sessionID: chat.id,
948+
agent: "build",
949+
command: "command -v __nonexistent_cmd_e2e__ || echo 'not found' >&2; exit 1",
950+
})
951+
952+
expect(result.info.role).toBe("assistant")
953+
const tool = completedTool(result.parts)
954+
if (!tool) return
955+
956+
expect(tool.state.output).toContain("not found")
957+
expect(tool.state.metadata.output).toContain("not found")
958+
yield* prompt.assertNotBusy(chat.id)
959+
}),
960+
{ git: true, config: cfg },
961+
),
962+
)
963+
941964
unix(
942965
"shell updates running metadata before process exit",
943966
() =>

0 commit comments

Comments
 (0)