refactor: replace BunProc with Npm module using @npmcli/arborist#18308
Merged
refactor: replace BunProc with Npm module using @npmcli/arborist#18308
Conversation
Contributor
Greptile SummaryThis PR replaces the Key issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Caller (formatter/plugin/lsp)
participant Npm as Npm module
participant Lock as Lock
participant Arborist as @npmcli/arborist
participant Registry as npm registry
participant FS as Filesystem
Note over Caller,FS: Npm.which(pkg)
Caller->>Npm: which(pkg)
Npm->>FS: readdir(.bin/)
FS-->>Npm: files[]
alt files.length == 1
Npm-->>Caller: binDir/files[0]
else files.length > 1
Npm->>FS: readJson(node_modules/pkg/package.json)
FS-->>Npm: { bin: string | Record }
Npm-->>Caller: binDir/resolvedName
else no files
Npm->>FS: rm(package-lock.json)
Npm->>Npm: add(pkg)
Note over Npm,Arborist: Npm.add(pkg)
Npm->>Lock: write(npm-install:pkg)
Npm->>Arborist: loadVirtual()
alt lockfile exists & node in tree
Arborist-->>Npm: tree (early return)
Npm-->>Caller: first.path ⚠️ no disk check
else fresh install
Npm->>Arborist: reify({add:[pkg]})
Arborist->>Registry: fetch package
Registry-->>Arborist: tarball
Arborist->>FS: extract to cache/packages/pkg/
Arborist-->>Npm: result tree
Npm-->>Caller: first.path
end
end
Note over Caller,FS: Npm.install(dir)
Caller->>Npm: install(dir)
Npm->>Lock: write(npm-install:dir)
Npm->>FS: exists(node_modules)
alt node_modules missing
Npm->>Arborist: reify() [errors silently swallowed]
else check lock vs package.json
Npm->>FS: readJson(package.json + package-lock.json)
alt dependency not in lockfile
Npm->>Arborist: reify() [errors silently swallowed]
end
end
Last reviewed commit: "fix: force reinstall..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
thdxr
force-pushed
the
refactor/npm-over-bunproc
branch
3 times, most recently
from
4dd891c to
d5bb559
Compare
thdxr
force-pushed
the
refactor/npm-over-bunproc
branch
from
d5bb559 to
ca2099e
Compare
Member
Author
|
@greptile try again |
thdxr
added a commit
that referenced
this pull request
Mar 20, 2026
Use npm-install:${pkg} instead of a global npm-install lock so
concurrent installs of different packages can run in parallel.
When the lockfile exists but .bin is empty or absent, add() would read the lockfile via loadVirtual() and return early without calling reify(). Delete the lockfile before calling add() so it proceeds with a full install.
Member
Author
|
@greptile |
Replace tsx dependency with explicit 'node' invocation for the compiled ESLint server. Remove all BUN_BE_BUN env var references which are no longer needed after the Bun-to-Node migration.
…interface The formatter Info interface uses enabled(): Promise<string[] | false> but user configs provide a command property. Added explicit return type to the arrow function that transforms command into the enabled() return value. Also added test to verify custom formatters with command property work.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Member
Author
Comment on lines
+157
to
+168
| // Multiple binaries — resolve from package.json bin field like npx does | ||
| const pkgJson = await Filesystem.readJson<{ bin?: string | Record<string, string> }>( | ||
| path.join(dir, "node_modules", pkg, "package.json"), | ||
| ).catch(() => undefined) | ||
| if (pkgJson?.bin) { | ||
| const unscoped = pkg.startsWith("@") ? pkg.split("/")[1] : pkg | ||
| const bin = pkgJson.bin | ||
| if (typeof bin === "string") return unscoped | ||
| const keys = Object.keys(bin) | ||
| if (keys.length === 1) return keys[0] | ||
| return bin[unscoped] ? unscoped : keys[0] | ||
| } |
Contributor
There was a problem hiding this comment.
unscoped referenced before its const declaration (TDZ ReferenceError)
On line 163 return unscoped references a variable that is declared with const on line 166. In JavaScript/TypeScript, const bindings are in the Temporal Dead Zone until their declaration is reached, so accessing unscoped here throws a ReferenceError at runtime.
This means Npm.which() will always crash for any installed package whose package.json declares bin as a bare string (e.g., "bin": "./cli.js") when the .bin directory contains more than one file.
Suggested change
| // Multiple binaries — resolve from package.json bin field like npx does | |
| const pkgJson = await Filesystem.readJson<{ bin?: string | Record<string, string> }>( | |
| path.join(dir, "node_modules", pkg, "package.json"), | |
| ).catch(() => undefined) | |
| if (pkgJson?.bin) { | |
| const unscoped = pkg.startsWith("@") ? pkg.split("/")[1] : pkg | |
| const bin = pkgJson.bin | |
| if (typeof bin === "string") return unscoped | |
| const keys = Object.keys(bin) | |
| if (keys.length === 1) return keys[0] | |
| return bin[unscoped] ? unscoped : keys[0] | |
| } | |
| if (pkgJson?.bin) { | |
| const bin = pkgJson.bin | |
| const unscoped = pkg.startsWith("@") ? pkg.split("/")[1] : pkg | |
| if (typeof bin === "string") return unscoped | |
| const keys = Object.keys(bin) | |
| if (keys.length === 1) return keys[0] | |
| return bin[unscoped] ? unscoped : keys[0] | |
| } |
Comment on lines
95
to
97
| return [await Npm.which("oxfmt"), "$FILE"] | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Npm.which("oxfmt") can throw uncaught inside enabled()
Unlike the prettier formatter (which wraps Npm.which in .catch(() => null)), oxfmt.enabled() calls await Npm.which("oxfmt") without any error handling. If the package fails to install (network error, InstallFailedError, etc.) the exception propagates up through isEnabled() in format/index.ts and breaks all subsequent formatting for the session.
Suggested change
| return [await Npm.which("oxfmt"), "$FILE"] | |
| } | |
| } | |
| return [await Npm.which("oxfmt").catch(() => null) ?? "", "$FILE"] |
Or better, bail out early if which fails:
const bin = await Npm.which("oxfmt").catch(() => null)
if (!bin) return false
return [bin, "$FILE"]
Contributor
|
This PR cannot be merged into the beta branch due to: Merge conflicts with dev branch Please resolve this issue to include this PR in the next beta release. |
- Replace BunProc.install with Npm.add for plugin resolution - Remove needsInstall check from installDependencies - Update test files to use Npm module instead of BunProc - Delete bun/index.ts and bun/registry.ts (no longer needed)
…refactor/npm-over-bunproc
Hona
reviewed
Mar 27, 2026
packages/opencode/src/npm/index.ts
Outdated
| // flag. See tar's get-write-flag.js. | ||
| // Must be set before @npmcli/arborist is imported since tar caches the flag | ||
| // at module evaluation time — so we use a dynamic import() below. | ||
| if (process.platform === "win32") { |
Member
There was a problem hiding this comment.
you can remove this now that we're on Bun 1.3.11 - my fix for this landed
Contributor
|
This PR cannot be merged into the beta branch due to: Merge conflicts with dev branch Please resolve this issue to include this PR in the next beta release. |
Contributor
|
This PR cannot be merged into the beta branch due to: Merge conflicts with dev branch Please resolve this issue to include this PR in the next beta release. |
2 similar comments
Contributor
|
This PR cannot be merged into the beta branch due to: Merge conflicts with dev branch Please resolve this issue to include this PR in the next beta release. |
Contributor
|
This PR cannot be merged into the beta branch due to: Merge conflicts with dev branch Please resolve this issue to include this PR in the next beta release. |
# Conflicts: # packages/opencode/src/bun/index.ts # packages/opencode/test/bun.test.ts
…st.ts - update resolvePluginTarget to use Npm.add instead of Npm.install - replace BunProc.install with Npm.add in provider.ts - update all tests to use Npm.add/Npm.install instead of BunProc
…tatic import instead of dynamic import
…ackage.json Npm.add was returning the package directory path, but dynamic import() requires the actual entry file path. Added resolveEntryPoint helper to read package.json and resolve the correct ESM entry point (via exports.import, module, or main fields) before returning the path.
…t from package.json" This reverts commit 0a8e3df.
Changed Npm.add() to return an object with both the package directory and resolved entrypoint. This allows callers to choose which they need - plugins use the directory for path resolution while providers use the entrypoint for loading.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/bun/(BunProc + PackageRegistry) — replaces allbun add/bun install/bun infocallssrc/npm/index.ts(Npm namespace) backed by@npmcli/arboristfor deterministic installsNpm.add(pkg)— install a single package into a per-package cache directoryNpm.install(dir)— reify dependencies in an existing project directoryNpm.which(pkg)— resolve the binary path for a cached packageNpm.outdated(pkg, version)— check npm registry for newer versionscommandremoved,enabled()now returnsstring[] | false), lsp/server.ts, plugin/index.ts, provider/provider.ts@npmcli/arborist+@types/npmcli__arboristto package.jsonConfidence Score: 1/5
formatter.tshas a structural brace mismatch that likely prevents the entire formatter module from loading, andnpm/index.tshas aconstTDZ crash inwhich(). Both issues are in hot code paths exercised at startup.packages/opencode/src/format/formatter.ts(brace mismatch) andpackages/opencode/src/npm/index.ts(TDZ bug on line 163) require immediate attention before this is mergeable.Important Files Changed
unscopedis used before itsconstdeclaration, crashingwhich()for packages with a stringbinfield in multi-binary scenarios.prettier.enabled(): duplicatedifcondition consumes the originalfor-loop close brace, leaving theprettierobject declaration unclosed and causing a parse/type error that likely prevents the entire formatter module from loading.nodedirectly (correct). Pyright now relies onNpm.which("pyright")which correctly resolves from package.jsonbin— if the TDZ bug innpm/index.tsis fixed.Npm.add(plugin)for plugin installation; error handling logs and skips failed plugins. Acceptable.BunProc.install(pkg, "latest")withNpm.add(pkg); note thatNpm.addno longer enforces the "latest" version constraint — any previously cached version is reused regardless of the requested version tag.BUN_BE_BUNenv var and addsas anycast to silence the TypeScript env type error. Minor but theas anysuppresses potentially legitimate type checking.Comments Outside Diff (1)
packages/opencode/src/format/formatter.ts, line 66-82 (link)prettier.enabled()— prettier object never closedThe refactor introduced a duplicated
ifblock with mismatched indentation, which shifts all the original closing braces one level inward. Counting the braces:if {if {}closes the innerif}(the originalfor-loop close) now closes the outerif}from},closes theforloop (leavingreturn falseon line 80 inside it)}closesenabled()function bodyThe
export const prettier: Info = {opened on line 36 has no matching closing brace, so TypeScript/Bun treats all subsequent formatter exports (oxfmt,biome, etc.) as properties of theprettierobject, producing a parse or type error.The fix is to remove the duplicated condition and restore the intended structure:
Last reviewed commit: "sync"