From 3c6c74457d53a01a3f42a758ad1317cd6ed1b963 Mon Sep 17 00:00:00 2001 From: Dax Raad Date: Wed, 25 Feb 2026 01:38:58 -0500 Subject: [PATCH] sync --- packages/opencode/BUN_SHELL_MIGRATION_PLAN.md | 136 ++++++++++++++++++ packages/opencode/src/util/git.ts | 74 +++------- packages/opencode/src/util/process.ts | 97 ++++++++++--- packages/opencode/test/util/process.test.ts | 59 ++++++++ 4 files changed, 294 insertions(+), 72 deletions(-) create mode 100644 packages/opencode/BUN_SHELL_MIGRATION_PLAN.md create mode 100644 packages/opencode/test/util/process.test.ts diff --git a/packages/opencode/BUN_SHELL_MIGRATION_PLAN.md b/packages/opencode/BUN_SHELL_MIGRATION_PLAN.md new file mode 100644 index 000000000..6cb21ac8f --- /dev/null +++ b/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. diff --git a/packages/opencode/src/util/git.ts b/packages/opencode/src/util/git.ts index 8e1427c99..731131357 100644 --- a/packages/opencode/src/util/git.ts +++ b/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 - stdout: Buffer | ReadableStream - stderr: Buffer | ReadableStream + 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 }): Promise { - if (Flag.OPENCODE_CLIENT === "acp") { - try { - const proc = Process.spawn(["git", ...args], { - stdin: "ignore", - stdout: "pipe", - stderr: "pipe", - cwd: opts.cwd, - env: opts.env ? { ...process.env, ...opts.env } : process.env, - }) - // 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 { - 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, - } + return Process.run(["git", ...args], { + cwd: opts.cwd, + env: opts.env, + stdin: "ignore", + nothrow: true, + }) + .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: Buffer.from(error instanceof Error ? error.message : String(error)), + })) } diff --git a/packages/opencode/src/util/process.ts b/packages/opencode/src/util/process.ts index 09c55661f..71f001a86 100644 --- a/packages/opencode/src/util/process.ts +++ b/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 { + 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 } - 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 | 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((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 { + 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) + } } diff --git a/packages/opencode/test/util/process.test.ts b/packages/opencode/test/util/process.test.ts new file mode 100644 index 000000000..ce599d6d8 --- /dev/null +++ b/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) +})