refactor(opencode): replace Bun shell in core flows#16286
Conversation
|
@greptile review |
…-opencode # Conflicts: # packages/opencode/src/file/index.ts # packages/opencode/src/worktree/index.ts
Greptile SummaryThis PR is a broad, mechanical refactoring that removes Bun's Key points:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant Process
participant NodeChildProcess
participant git helper
Note over Caller,git helper: New Process.text() / Process.lines() flow
Caller->>Process: text(cmd, opts)
Process->>Process: run(cmd, opts)
Process->>NodeChildProcess: spawn(cmd[0], cmd[1..], {stdout:pipe, stderr:pipe})
NodeChildProcess-->>Process: ChildProcess
Process->>NodeChildProcess: buffer(stdout), buffer(stderr), exited
NodeChildProcess-->>Process: [code, stdout, stderr]
alt nothrow=true and code≠0
Process-->>Caller: {code, stdout, stderr, text}
else code=0
Process-->>Caller: {code, stdout, stderr, text}
else nothrow=false and code≠0
Process-->>Caller: throws RunFailedError
end
Note over Caller,git helper: git() helper (always nothrow)
Caller->>git helper: git(args, {cwd})
git helper->>Process: run(["git",...args], {stdin:ignore, nothrow:true})
Process->>NodeChildProcess: spawn("git", args, {stdin:ignore})
NodeChildProcess-->>Process: {code, stdout, stderr}
git helper-->>Caller: {exitCode, text(), stdout, stderr}
Note over Caller,git helper: upgradeCurl — fetch+bash stdin pipe
Caller->>Process: spawn(["bash"], {stdin:pipe, stdout:pipe, stderr:pipe})
Process->>NodeChildProcess: launch("bash", [], {stdio:[pipe,pipe,pipe]})
Caller->>NodeChildProcess: stdin.end(install_script_body)
NodeChildProcess-->>Caller: {code, stdout, stderr}
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptile |
| export interface TextResult extends Result { | ||
| text: string | ||
| } |
There was a problem hiding this comment.
TextResult.text (property) vs GitResult.text() (method) — inconsistent API surface across the codebase
Process.text() returns TextResult where text is a plain string property, while util/git.ts returns GitResult where text is a zero-arg method (text()). This means callers accessing stdout text must choose between result.text and result.text() depending on which helper they used. The mixed surface makes it easy to accidentally call result.text as a property when the caller is holding a GitResult (getting the function reference rather than the string), or vice-versa.
Consider aligning the two shapes — either both expose text as a property, or both expose it as a method — to make the API uniform across the codebase. Since GitResult appears in the older helper and the majority of call sites, making TextResult.text a method (or aliasing both) would be the least disruptive change.
|
@greptile what up |
|
Hey! Nothing much, just here to help with the review. Did you have a question about the refactoring or anything specific you wanted to discuss? |
|
@greptile |
There was a problem hiding this comment.
Pull request overview
Refactors core packages/opencode/src flows to remove Bun $ shell usage in favor of argv-based process execution (Process) and a dedicated git() helper, improving portability and reducing reliance on shell parsing.
Changes:
- Added
Process.text()andProcess.lines()helpers to complementProcess.run(). - Migrated many git/process operations (worktree, snapshot, file, installation, clipboard, LSP installs, CLI commands) off Bun
$toProcess/git()and Node FS APIs. - Replaced a few shell one-offs with Node equivalents (
fs.realpath,fs.rm,fs.chmod, directory scanning viafs.readdir, downloads viafetch+Filesystem.writeStream).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/src/worktree/index.ts | Migrates git commands and start-command execution to git()/Process.run(). |
| packages/opencode/src/util/process.ts | Adds text() and lines() helpers and improves nothrow handling in run(). |
| packages/opencode/src/util/archive.ts | Replaces unzip/PowerShell extraction shell usage with Process.run(). |
| packages/opencode/src/tool/bash.ts | Replaces realpath shell call with fs.realpath(). |
| packages/opencode/src/storage/storage.ts | Migrates git usage to git() (introduces a critical destructuring bug). |
| packages/opencode/src/snapshot/index.ts | Replaces Bun $ git flows with Process.run/text/lines. |
| packages/opencode/src/project/vcs.ts | Uses git() to read current branch instead of Bun shell. |
| packages/opencode/src/lsp/server.ts | Replaces multiple install/build shell invocations with Process + Node FS + fetch. |
| packages/opencode/src/installation/index.ts | Reworks upgrade/check flows to Process and implements `curl |
| packages/opencode/src/file/watcher.ts | Uses git() to locate .git directory. |
| packages/opencode/src/file/ripgrep.ts | Switches from raw shell command to argv execution via Process.text(). |
| packages/opencode/src/file/index.ts | Migrates git diff/list/show calls to git(). |
| packages/opencode/src/cli/cmd/uninstall.ts | Replaces uninstall shell execution with Process.run(). |
| packages/opencode/src/cli/cmd/tui/util/clipboard.ts | Migrates clipboard shell calls to Process + Node FS. |
| packages/opencode/src/cli/cmd/pr.ts | Migrates gh/git usage to Process + git(). |
| packages/opencode/src/cli/cmd/github.ts | Migrates git shell usage to git() and updates error handling to Process.RunFailedError. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function text(cmd: string[], opts: RunOptions = {}): Promise<TextResult> { | ||
| const out = await run(cmd, opts) | ||
| return { | ||
| ...out, | ||
| text: out.stdout.toString(), | ||
| } | ||
| } | ||
|
|
||
| export async function lines(cmd: string[], opts: RunOptions = {}): Promise<string[]> { | ||
| return (await text(cmd, opts)).text.split(/\r?\n/).filter(Boolean) | ||
| } |
There was a problem hiding this comment.
Process.text() / Process.lines() are newly added but there are no unit tests covering them (including behavior when nothrow is set and the command fails). Since packages/opencode/test/util/process.test.ts already exists for Process.run(), please add coverage for these helpers as well to lock in their expected semantics (newline splitting, encoding, and error behavior).
| for (const line of await Process.lines( | ||
| [ | ||
| "git", | ||
| "-c", | ||
| "core.autocrlf=false", | ||
| "-c", | ||
| "core.longpaths=true", | ||
| "-c", | ||
| "core.symlinks=true", | ||
| "-c", | ||
| "core.quotepath=false", | ||
| ...args(git, ["diff", "--no-ext-diff", "--no-renames", "--numstat", from, to, "--", "."]), | ||
| ], | ||
| { | ||
| cwd: Instance.directory, | ||
| nothrow: true, | ||
| }, | ||
| )) { |
There was a problem hiding this comment.
This change switches from streaming line iteration (previously for await (...) over Bun’s .lines()) to await Process.lines(...), which buffers the entire command output into memory before processing. For large diffs, this can increase peak memory and delay first-item processing. Consider using a streaming approach (e.g., spawn + readline) or adding a Process.iterLines() async iterator helper to preserve the previous behavior.
| const result = await git(["rev-list", "--max-parents=0", "--all"], { | ||
| cwd: worktree, | ||
| }) | ||
| const [id] = result | ||
| .text() | ||
| .then((x) => | ||
| x | ||
| .split("\n") | ||
| .filter(Boolean) | ||
| .map((x) => x.trim()) | ||
| .toSorted(), | ||
| ) | ||
| .split("\n") | ||
| .filter(Boolean) | ||
| .map((x) => x.trim()) | ||
| .toSorted() |
There was a problem hiding this comment.
result is a GitResult object and isn’t iterable. const [id] = result will throw (or fail typecheck). This should destructure from the output string, e.g. const [id] = result.text().split(...) (or assign const ids = ...).
| const body = await fetch("https://opencode.ai/install").then((res) => { | ||
| if (!res.ok) throw new Error(res.statusText) | ||
| return res.text() | ||
| }) | ||
| const proc = Process.spawn(["bash"], { | ||
| stdin: "pipe", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| env: { | ||
| ...process.env, | ||
| VERSION: target, | ||
| }, | ||
| }) | ||
| if (!proc.stdin || !proc.stdout || !proc.stderr) throw new Error("Process output not available") | ||
| proc.stdin.end(body) | ||
| const [code, stdout, stderr] = await Promise.all([proc.exited, buffer(proc.stdout), buffer(proc.stderr)]) | ||
| return { | ||
| code, | ||
| stdout, | ||
| stderr, |
There was a problem hiding this comment.
upgradeCurl() can throw (e.g. network failure, bash missing/spawn error, stream read error). Unlike the other branches (which use Process.run(..., { nothrow: true })), these throws bypass the result.code handling and won’t be wrapped into UpgradeFailedError. Consider catching inside upgradeCurl (or around the call site) and returning a { code: 1, stdout, stderr }-shaped result (or rethrowing as UpgradeFailedError) to keep error handling consistent across methods.
| const body = await fetch("https://opencode.ai/install").then((res) => { | |
| if (!res.ok) throw new Error(res.statusText) | |
| return res.text() | |
| }) | |
| const proc = Process.spawn(["bash"], { | |
| stdin: "pipe", | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| env: { | |
| ...process.env, | |
| VERSION: target, | |
| }, | |
| }) | |
| if (!proc.stdin || !proc.stdout || !proc.stderr) throw new Error("Process output not available") | |
| proc.stdin.end(body) | |
| const [code, stdout, stderr] = await Promise.all([proc.exited, buffer(proc.stdout), buffer(proc.stderr)]) | |
| return { | |
| code, | |
| stdout, | |
| stderr, | |
| try { | |
| const body = await fetch("https://opencode.ai/install").then((res) => { | |
| if (!res.ok) throw new Error(res.statusText) | |
| return res.text() | |
| }) | |
| const proc = Process.spawn(["bash"], { | |
| stdin: "pipe", | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| env: { | |
| ...process.env, | |
| VERSION: target, | |
| }, | |
| }) | |
| if (!proc.stdin || !proc.stdout || !proc.stderr) throw new Error("Process output not available") | |
| proc.stdin.end(body) | |
| const [code, stdout, stderr] = await Promise.all([proc.exited, buffer(proc.stdout), buffer(proc.stderr)]) | |
| return { | |
| code, | |
| stdout, | |
| stderr, | |
| } | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error) | |
| return { | |
| code: 1, | |
| stdout: Buffer.from(""), | |
| stderr: Buffer.from(message), | |
| } |
| @@ -8,9 +8,10 @@ export namespace Archive { | |||
| const winDestDir = path.resolve(destDir) | |||
| // $global:ProgressPreference suppresses PowerShell's blue progress bar popup | |||
| const cmd = `$global:ProgressPreference = 'SilentlyContinue'; Expand-Archive -Path '${winZipPath}' -DestinationPath '${winDestDir}' -Force` | |||
| await $`powershell -NoProfile -NonInteractive -Command ${cmd}`.quiet() | |||
| } else { | |||
| await $`unzip -o -q ${zipPath} -d ${destDir}`.quiet() | |||
| await Process.run(["powershell", "-NoProfile", "-NonInteractive", "-Command", cmd]) | |||
| return | |||
| } | |||
|
|
|||
| await Process.run(["unzip", "-o", "-q", zipPath, "-d", destDir]) | |||
| } | |||
There was a problem hiding this comment.
No error handling / nothrow on extractZip
Both Process.run calls use default options (no nothrow: true), so a non-zero exit from powershell or unzip will throw a RunFailedError. The original Bun $ calls also lacked .nothrow(), so the throwing behaviour is unchanged — however, one call site (lsp/server.ts) catches the error via .catch() while other call sites (also in lsp/server.ts) do not. Worth verifying all callers intentionally tolerate exceptions from this function.
Summary
Process.text()andProcess.lines()so argv-based process execution can replace Bun shell text and line helperspackages/opencode/srcaway from Bun$fs.realpath()andfs.rm()Testing
bun run typecheck