sync

packages/opencode/BUN_SHELL_MIGRATION_PLAN.md

@@ -0,0 +1,136 @@
+# Bun shell migration plan
+
+Practical phased replacement of Bun `$` calls.
+
+## Goal
+
+Replace runtime Bun shell template-tag usage in `packages/opencode/src` with a unified `Process` API in `util/process.ts`.
+
+Keep behavior stable while improving safety, testability, and observability.
+
+Current baseline from audit:
+
+- 143 runtime command invocations across 17 files
+- 84 are git commands
+- Largest hotspots:
+  - `src/cli/cmd/github.ts` (33)
+  - `src/worktree/index.ts` (22)
+  - `src/lsp/server.ts` (21)
+  - `src/installation/index.ts` (20)
+  - `src/snapshot/index.ts` (18)
+
+## Decisions
+
+- Extend `src/util/process.ts` (do not create a separate exec module).
+- Proceed with phased migration for both git and non-git paths.
+- Keep plugin `$` compatibility in 1.x and remove in 2.0.
+
+## Non-goals
+
+- Do not remove plugin `$` compatibility in this effort.
+- Do not redesign command semantics beyond what is needed to preserve behavior.
+
+## Constraints
+
+- Keep migration phased, not big-bang.
+- Minimize behavioral drift.
+- Keep these explicit shell-only exceptions:
+  - `src/session/prompt.ts` raw command execution
+  - worktree start scripts in `src/worktree/index.ts`
+
+## Process API proposal (`src/util/process.ts`)
+
+Add higher-level wrappers on top of current spawn support.
+
+Core methods:
+
+- `Process.run(cmd, opts)`
+- `Process.text(cmd, opts)`
+- `Process.lines(cmd, opts)`
+- `Process.status(cmd, opts)`
+- `Process.shell(command, opts)` for intentional shell execution
+
+Git helpers:
+
+- `Process.git(args, opts)`
+- `Process.gitText(args, opts)`
+
+Shared options:
+
+- `cwd`, `env`, `stdin`, `stdout`, `stderr`, `abort`, `timeout`, `kill`
+- `allowFailure` / non-throw mode
+- optional redaction + trace metadata
+
+Standard result shape:
+
+- `code`, `stdout`, `stderr`, `duration_ms`, `cmd`
+- helpers like `text()` and `arrayBuffer()` where useful
+
+## Phased rollout
+
+### Phase 0: Foundation
+
+- Implement Process wrappers in `src/util/process.ts`.
+- Refactor `src/util/git.ts` to use Process only.
+- Add tests for exit handling, timeout, abort, and output capture.
+
+### Phase 1: High-impact hotspots
+
+Migrate these first:
+
+- `src/cli/cmd/github.ts`
+- `src/worktree/index.ts`
+- `src/lsp/server.ts`
+- `src/installation/index.ts`
+- `src/snapshot/index.ts`
+
+Within each file, migrate git paths first where applicable.
+
+### Phase 2: Remaining git-heavy files
+
+Migrate git-centric call sites to `Process.git*` helpers:
+
+- `src/file/index.ts`
+- `src/project/vcs.ts`
+- `src/file/watcher.ts`
+- `src/storage/storage.ts`
+- `src/cli/cmd/pr.ts`
+
+### Phase 3: Remaining non-git files
+
+Migrate residual non-git usages:
+
+- `src/cli/cmd/tui/util/clipboard.ts`
+- `src/util/archive.ts`
+- `src/file/ripgrep.ts`
+- `src/tool/bash.ts`
+- `src/cli/cmd/uninstall.ts`
+
+### Phase 4: Stabilize
+
+- Remove dead wrappers and one-off patterns.
+- Keep plugin `$` compatibility isolated and documented as temporary.
+- Create linked 2.0 task for plugin `$` removal.
+
+## Validation strategy
+
+- Unit tests for new `Process` methods and options.
+- Integration tests on hotspot modules.
+- Smoke tests for install, snapshot, worktree, and GitHub flows.
+- Regression checks for output parsing behavior.
+
+## Risk mitigation
+
+- File-by-file PRs with small diffs.
+- Preserve behavior first, simplify second.
+- Keep shell-only exceptions explicit and documented.
+- Add consistent error shaping and logging at Process layer.
+
+## Definition of done
+
+- Runtime Bun `$` usage in `packages/opencode/src` is removed except:
+  - approved shell-only exceptions
+  - temporary plugin compatibility path (1.x)
+- Git paths use `Process.git*` consistently.
+- CI and targeted smoke tests pass.
+- 2.0 issue exists for plugin `$` removal.

packages/opencode/src/util/git.ts

@@ -1,63 +1,35 @@
-import { $ } from "bun"
-import { buffer } from "node:stream/consumers"
-import { Flag } from "../flag/flag"
 import { Process } from "./process"
 
 export interface GitResult {
   exitCode: number
-  text(): string | Promise<string>
-  stdout: Buffer | ReadableStream<Uint8Array>
-  stderr: Buffer | ReadableStream<Uint8Array>
+  text(): string
+  stdout: Buffer
+  stderr: Buffer
 }
 
 /**
  * Run a git command.
  *
- * Uses Bun's lightweight `$` shell by default.  When the process is running
- * as an ACP client, child processes inherit the parent's stdin pipe which
- * carries protocol data – on Windows this causes git to deadlock.  In that
- * case we fall back to `Process.spawn` with `stdin: "ignore"`.
+ * Uses Process helpers with stdin ignored to avoid protocol pipe inheritance
+ * issues in embedded/client environments.
  */
 export async function git(args: string[], opts: { cwd: string; env?: Record<string, string> }): Promise<GitResult> {
-  if (Flag.OPENCODE_CLIENT === "acp") {
-    try {
-      const proc = Process.spawn(["git", ...args], {
-        stdin: "ignore",
-        stdout: "pipe",
-        stderr: "pipe",
+  return Process.run(["git", ...args], {
     cwd: opts.cwd,
-        env: opts.env ? { ...process.env, ...opts.env } : process.env,
+    env: opts.env,
+    stdin: "ignore",
+    nothrow: true,
   })
-      // Read output concurrently with exit to avoid pipe buffer deadlock
-      if (!proc.stdout || !proc.stderr) {
-        throw new Error("Process output not available")
-      }
-      const [exitCode, out, err] = await Promise.all([proc.exited, buffer(proc.stdout), buffer(proc.stderr)])
-      return {
-        exitCode,
-        text: () => out.toString(),
-        stdout: out,
-        stderr: err,
-      }
-    } catch (error) {
-      const stderr = Buffer.from(error instanceof Error ? error.message : String(error))
-      return {
+    .then((result) => ({
+      exitCode: result.code,
+      text: () => result.stdout.toString(),
+      stdout: result.stdout,
+      stderr: result.stderr,
+    }))
+    .catch((error) => ({
       exitCode: 1,
       text: () => "",
       stdout: Buffer.alloc(0),
-        stderr,
-      }
-    }
-  }
-
-  const env = opts.env ? { ...process.env, ...opts.env } : undefined
-  let cmd = $`git ${args}`.quiet().nothrow().cwd(opts.cwd)
-  if (env) cmd = cmd.env(env)
-  const result = await cmd
-  return {
-    exitCode: result.exitCode,
-    text: () => result.text(),
-    stdout: result.stdout,
-    stderr: result.stderr,
-  }
+      stderr: Buffer.from(error instanceof Error ? error.message : String(error)),
+    }))
 }

packages/opencode/src/util/process.ts

@@ -1,4 +1,5 @@
 import { spawn as launch, type ChildProcess } from "child_process"
+import { buffer } from "node:stream/consumers"
 
 export namespace Process {
   export type Stdio = "inherit" | "pipe" | "ignore"
@@ -14,58 +15,112 @@ export namespace Process {
     timeout?: number
   }
 
+  export interface RunOptions extends Omit<Options, "stdout" | "stderr"> {
+    nothrow?: boolean
+  }
+
+  export interface Result {
+    code: number
+    stdout: Buffer
+    stderr: Buffer
+  }
+
+  export class RunFailedError extends Error {
+    readonly cmd: string[]
+    readonly code: number
+    readonly stdout: Buffer
+    readonly stderr: Buffer
+
+    constructor(cmd: string[], code: number, stdout: Buffer, stderr: Buffer) {
+      const text = stderr.toString().trim()
+      super(
+        text
+          ? `Command failed with code ${code}: ${cmd.join(" ")}\n${text}`
+          : `Command failed with code ${code}: ${cmd.join(" ")}`,
+      )
+      this.name = "ProcessRunFailedError"
+      this.cmd = [...cmd]
+      this.code = code
+      this.stdout = stdout
+      this.stderr = stderr
+    }
+  }
+
   export type Child = ChildProcess & { exited: Promise<number> }
 
-  export function spawn(cmd: string[], options: Options = {}): Child {
+  export function spawn(cmd: string[], opts: Options = {}): Child {
     if (cmd.length === 0) throw new Error("Command is required")
-    options.abort?.throwIfAborted()
+    opts.abort?.throwIfAborted()
 
     const proc = launch(cmd[0], cmd.slice(1), {
-      cwd: options.cwd,
-      env: options.env === null ? {} : options.env ? { ...process.env, ...options.env } : undefined,
-      stdio: [options.stdin ?? "ignore", options.stdout ?? "ignore", options.stderr ?? "ignore"],
+      cwd: opts.cwd,
+      env: opts.env === null ? {} : opts.env ? { ...process.env, ...opts.env } : undefined,
+      stdio: [opts.stdin ?? "ignore", opts.stdout ?? "ignore", opts.stderr ?? "ignore"],
     })
 
-    let aborted = false
+    let closed = false
     let timer: ReturnType<typeof setTimeout> | undefined
 
     const abort = () => {
-      if (aborted) return
+      if (closed) return
       if (proc.exitCode !== null || proc.signalCode !== null) return
-      aborted = true
+      closed = true
 
-      proc.kill(options.kill ?? "SIGTERM")
+      proc.kill(opts.kill ?? "SIGTERM")
 
-      const timeout = options.timeout ?? 5_000
-      if (timeout <= 0) return
-
-      timer = setTimeout(() => {
-        proc.kill("SIGKILL")
-      }, timeout)
+      const ms = opts.timeout ?? 5_000
+      if (ms <= 0) return
+      timer = setTimeout(() => proc.kill("SIGKILL"), ms)
     }
 
     const exited = new Promise<number>((resolve, reject) => {
       const done = () => {
-        options.abort?.removeEventListener("abort", abort)
+        opts.abort?.removeEventListener("abort", abort)
         if (timer) clearTimeout(timer)
       }
-      proc.once("exit", (exitCode, signal) => {
+
+      proc.once("exit", (code, signal) => {
         done()
-        resolve(exitCode ?? (signal ? 1 : 0))
+        resolve(code ?? (signal ? 1 : 0))
       })
+
       proc.once("error", (error) => {
         done()
         reject(error)
       })
     })
 
-    if (options.abort) {
-      options.abort.addEventListener("abort", abort, { once: true })
-      if (options.abort.aborted) abort()
+    if (opts.abort) {
+      opts.abort.addEventListener("abort", abort, { once: true })
+      if (opts.abort.aborted) abort()
     }
 
     const child = proc as Child
     child.exited = exited
     return child
   }
+
+  export async function run(cmd: string[], opts: RunOptions = {}): Promise<Result> {
+    const proc = spawn(cmd, {
+      cwd: opts.cwd,
+      env: opts.env,
+      stdin: opts.stdin,
+      abort: opts.abort,
+      kill: opts.kill,
+      timeout: opts.timeout,
+      stdout: "pipe",
+      stderr: "pipe",
+    })
+
+    if (!proc.stdout || !proc.stderr) throw new Error("Process output not available")
+
+    const [code, stdout, stderr] = await Promise.all([proc.exited, buffer(proc.stdout), buffer(proc.stderr)])
+    const out = {
+      code,
+      stdout,
+      stderr,
+    }
+    if (out.code === 0 || opts.nothrow) return out
+    throw new RunFailedError(cmd, out.code, out.stdout, out.stderr)
+  }
 }

packages/opencode/test/util/process.test.ts

@@ -0,0 +1,59 @@
+import { describe, expect, test } from "bun:test"
+import { Process } from "../../src/util/process"
+
+function node(script: string) {
+  return [process.execPath, "-e", script]
+}
+
+describe("util.process", () => {
+  test("captures stdout and stderr", async () => {
+    const out = await Process.run(node('process.stdout.write("out");process.stderr.write("err")'))
+    expect(out.code).toBe(0)
+    expect(out.stdout.toString()).toBe("out")
+    expect(out.stderr.toString()).toBe("err")
+  })
+
+  test("returns code when nothrow is enabled", async () => {
+    const out = await Process.run(node("process.exit(7)"), { nothrow: true })
+    expect(out.code).toBe(7)
+  })
+
+  test("throws RunFailedError on non-zero exit", async () => {
+    const err = await Process.run(node('process.stderr.write("bad");process.exit(3)')).catch((error) => error)
+    expect(err).toBeInstanceOf(Process.RunFailedError)
+    if (!(err instanceof Process.RunFailedError)) throw err
+    expect(err.code).toBe(3)
+    expect(err.stderr.toString()).toBe("bad")
+  })
+
+  test("aborts a running process", async () => {
+    const abort = new AbortController()
+    const started = Date.now()
+    setTimeout(() => abort.abort(), 25)
+
+    const out = await Process.run(node("setInterval(() => {}, 1000)"), {
+      abort: abort.signal,
+      nothrow: true,
+    })
+
+    expect(out.code).not.toBe(0)
+    expect(Date.now() - started).toBeLessThan(1000)
+  }, 3000)
+
+  test("kills after timeout when process ignores terminate signal", async () => {
+    if (process.platform === "win32") return
+
+    const abort = new AbortController()
+    const started = Date.now()
+    setTimeout(() => abort.abort(), 25)
+
+    const out = await Process.run(node('process.on("SIGTERM", () => {}); setInterval(() => {}, 1000)'), {
+      abort: abort.signal,
+      nothrow: true,
+      timeout: 25,
+    })
+
+    expect(out.code).not.toBe(0)
+    expect(Date.now() - started).toBeLessThan(1000)
+  }, 3000)
+})