Skip to content

run: shim npm/npx alongside node in the --bun PATH dir - #35474

Open
robobun wants to merge 23 commits into
mainfrom
farm/311a28c7/npm-npx-shim
Open

robobun wants to merge 23 commits into
mainfrom
farm/311a28c7/npm-npx-shim

Conversation

@robobun

@robobun robobun commented Jul 24, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #5995
Fixes #14773

Repro

On a host with no npm/npx in PATH:

$ bun create react-app my-app
...
TypeError: Executable not found in $PATH: "npm"
      at spawn (node:child_process:721:8)
      at spawn (.../cross-spawn/index.js:12:24)
      at .../create-react-app/createReactApp.js:383:19

Minimal form:

// package.json: { "scripts": { "go": "node check.js" } }
// check.js:
require('child_process').spawnSync('npm', ['--version']); // ENOENT

bun --bun run go (or bun run go on a host without node) fails to find npm.

Cause

When --bun is passed or node is not in PATH, RunCommand::create_fake_temporary_node_executable (src/install/lib.rs) creates a temp dir with node and bun links at the running bun binary and prepends it to PATH, so scripts that spawn node get bun. Scripts that spawn npm or npx via child_process (as create-react-app and many scaffolding tools do) still see ENOENT on a bun-only host. The existing replace_package_manager_run rewrite only touches package.json script strings, not spawn('npm', ...) calls from JavaScript.

Fix

Add npm and npx links to the same shim dir, only when they are not already resolvable in the original PATH. A host with a real npm is unaffected (--bun keeps meaning "symlink node", nothing more); a bun-only host gets a best-effort shim instead of ENOENT. A stale shim link left by a previous run is removed when a real npm is present so it can never shadow.

Teach the CLI entry point to recognize those argv0 basenames:

  • argv0 == npx dispatches as bunx.
  • argv0 == npm rewrites argv into the equivalent bun invocation and falls through to the normal subcommand matcher:
    • npm lifecycle shortcuts test/t/tst/start/stop/restart become run <name>, so npm test runs the package.json test script rather than bun's test runner.
    • npm exec dispatches as bun x (bun's own exec is a shell-script runner); npm init <x>/npm create <x> dispatch as bun create <x>; bare npm init stays bun init; npm version dispatches as bun pm version; npm's install/ci/run/ls/view aliases are mapped.
    • npm-only value-taking options (--loglevel, --userconfig, --script-shell, ...) are dropped together with their value so the value is not mis-parsed as a positional. Otherwise npm install --no-audit --save --save-exact --loglevel error react (create-react-app's exact invocation) would try to install a package named error.
    • --workspace/-w is translated to --filter and --prefix/-C to --cwd, hoisted after the mapped subcommand so bun run's stop-after-first-positional does not pass them through to the script. --tag/--access/--otp are kept for publish and --message for version, where bun accepts them.

The basename check is exact so pnpm/pnpx are not caught. Unknown npm boolean flags are silently dropped as before, and subcommands with no bun equivalent pass through unchanged.

Verification

New test/cli/run/as-npm.test.ts:

  • spawn('npm'|'npx', ['--version']) from inside a --bun run script with no npm in PATH resolves via the shim (the bun create react-app fails unless Node is installed #5995 failure)
  • with a real npm/npx on PATH, --bun run does not shadow it
  • npm test runs the package.json script, not bun's test runner
  • npm install --no-audit --save --save-exact --loglevel error --logs-dir silent --dry-run runs as bun install with no positionals
  • npx --help / npm exec --help show bunx usage
  • npm init <x> / npm create <x> dispatch as bun create, bare npm init as bun init
  • npm run go -w pkg / --workspace=pkg / --prefix dir reach the right workspace
  • -- stops flag translation
  • pnpm as argv0 is not translated

Six of the eleven tests fail on the current binary; all pass with the change. test/cli/run/as-node.test.ts and test/cli/install/bun-run.test.ts are unchanged and continue to pass.


no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/as-npm.test.ts

When bun runs a script with --bun (or when node is not in PATH), it
creates a temp dir with a 'node' symlink pointing at bun and prepends
it to PATH, so tools that spawn 'node' get bun. Tools that spawn 'npm'
or 'npx' (create-react-app and many others do this via child_process,
not via a package.json script string that replace_package_manager_run
could rewrite) still fail with ENOENT on a host without npm.

Add 'npm' and 'npx' links to the same shim dir. When bun is invoked
with argv0 basename 'npx', dispatch as bunx. When invoked as 'npm',
translate argv to a bun-compatible shape before normal dispatch:

  - map npm lifecycle shortcuts (test/start/stop/restart) to
    'run <name>' so 'npm test' runs the package.json script, not
    bun's test runner
  - map npm subcommand aliases bun does not recognize
  - drop npm-only value-taking options (loglevel/prefix/userconfig/...)
    together with their value, so e.g.
    'npm install --loglevel error react' does not try to install a
    package called 'error'

The basename check is exact so 'pnpm'/'pnpx' are not caught.

Fixes #5995
@coderabbitai

coderabbitai Bot commented Jul 24, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3ad26728-e18f-458c-a3ed-8abe5b27cd8e

📥 Commits

Reviewing files that changed from the base of the PR and between 466c7fa and 564ecc1.

📒 Files selected for processing (3)
  • src/install/lib.rs
  • src/runtime/cli/mod.rs
  • test/cli/run/as-npm.test.ts

Walkthrough

Changes

The PR adds PATH-aware npm/npx shims, expands npm/npx argv rewriting and dispatch, adjusts startup and create parsing, and adds integration coverage for command routing, flag translation, and shim precedence.

npm and npx emulation

Layer / File(s) Summary
PATH-aware npm and npx shims
src/install/PackageManager.rs, src/install/lib.rs, src/runtime/cli/run_command.rs
Fake npm and npx links or hardlinks are created only when absent from the original PATH, with platform-specific stale executable handling.
npm and npx argv dispatch
src/runtime/cli/mod.rs
npm and npx argv are rewritten and routed to Bun equivalents, with updated startup fast paths and create argument parsing.
npm and npx integration coverage
test/cli/run/as-npm.test.ts
Tests cover PATH resolution, shim precedence, argv0 dispatch, flag translation, workspace and prefix routing, create behavior, and pnpm separation.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address both linked issues by shimming npm/npx when absent and adding npm/create-react-app dispatch coverage.
Out of Scope Changes check ✅ Passed The added CLI translation and tests are aligned with the npm/npx shim objectives and do not appear unrelated.
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding npm/npx shims alongside node in the --bun PATH dir.
Description check ✅ Passed The description includes the problem, cause, fix, and verification details, which covers the required content despite different headings.

Comment @coderabbitai help to get the list of available commands.

Comment thread src/runtime/cli/mod.rs Outdated
Comment thread src/runtime/cli/mod.rs Outdated
Comment thread test/cli/run/as-npm.test.ts Outdated
Comment thread test/cli/run/as-npm.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 src/runtime/cli/mod.rs:1118-1122 — The is_npx branch dispatches straight to Tag::BunxCommand without running any equivalent of copy_dropping_npm_flags, so spawnSync('npx', ['--loglevel', 'error', 'create-react-app', ...]) reaches bunx's Options::parse, which silently ignores the unknown --loglevel token but treats its value error as the package name. Since npm 7, npx accepts the same config flags this PR filters for npm — worth running NPM_VALUE_FLAGS_IGNORED (plus -c/--call) over argv here too, or teaching bunx's parser to skip them, so the sibling entry point gets the same fix.

    Extended reasoning...

    What the bug is

    The PR's own description states the fix: "npm-only value-taking options (--loglevel, --prefix, --userconfig, ...) are dropped together with their value so the value is not mis-parsed as a positional. Otherwise npm install ... --loglevel error react would try to install a package named error." That drop is implemented in translate_npm_argv() / copy_dropping_npm_flags, but only the is_npm(argv0) branch at mod.rs:1124 reaches it.

    The sibling is_npx(argv0) branch at mod.rs:1118-1122 — added in this same PR — sets IS_BUNX_EXE and returns Tag::BunxCommand immediately, with no argv rewrite. Since npm 7, npx is documented as a thin wrapper over npm exec and accepts every npm config option, including the exact space-separated --loglevel, --prefix, --cache, --userconfig flags that NPM_VALUE_FLAGS_IGNORED filters. It also accepts npx-specific value flags like -c / --call.

    Step-by-step trace

    A tool inside a --bun run script does spawnSync('npx', ['--loglevel', 'error', 'create-react-app', 'my-app']):

    1. The shim dir resolves npx → the bun binary. is_npx(argv0) at mod.rs:1118 → true → IS_BUNX_EXE = true, return Tag::BunxCommand. No argv translation runs.
    2. exec_bunx() at mod.rs:1717: IS_BUNX_EXE is true → start_idx = 0, so Options::parse receives ["<path>/npx", "--loglevel", "error", "create-react-app", "my-app"].
    3. Options::parse (bunx_command.rs:96-163):
      • i=0: "<path>/npx" doesn't start with - → found_subcommand_name = true.
      • i=1: "--loglevel" starts with - but matches none of the ~10 hardcoded flags (--version/--revision/--verbose/--silent/--bun/--no-install/--package/-p) → falls off the else-if chain silently, i += 1.
      • i=2: "error" doesn't start with -, and found_subcommand_name is already true → maybe_package_name = Some(b"error").
      • i=3+: maybe_package_name.is_some() → pushed to passthrough_list.
    4. opts.package_name = b"error". bunx resolves/installs and executes a package named error, passing create-react-app my-app as its arguments.

    The identical failure applies to npx --prefix <dir> <pkg> (runs package <dir>), npx --cache <dir> <pkg>, and npx -c "<cmd>" / npx --call "<cmd>" (runs a package whose name is the shell command string).

    Why nothing prevents it

    copy_dropping_npm_flags and NPM_VALUE_FLAGS_IGNORED are only reachable from translate_npm_argv(), which is gated on is_npm. The is_npx path added in the same commit does not share the flag-drop logic. bunx's own parser has no catch-all for unknown value-taking flags — it silently ignores the flag token but not its separate value argument, which is exactly the failure mode the PR's flag-dropper was built to prevent.

    Impact & severity

    Marked nit. Before this PR the same invocation on a bun-only host was ENOENT (loud, correct diagnosis), so this is not a regression from working behavior; the shim is explicitly best-effort/opt-in; and space-separated npm config flags before the package name in npx invocations are considerably rarer than in the npm install shape create-react-app uses. But it is precisely the same-class bug the PR's own rationale identifies ("would try to install a package named error") on the sibling entry point the same PR introduces. REVIEW.md: "Fix the whole class in the same PR — every sibling entry point receiving the same fix."

    Fix

    Either run the same value-flag drop over argv in the is_npx(argv0) branch before returning Tag::BunxCommand (a small helper that walks argv[1..] applying is_npm_ignored_value_flag + drops -c/--call/-y/--yes), or extend bunx_command::Options::parse to recognize-and-skip the entries in NPM_VALUE_FLAGS_IGNORED together with their next-arg value. The former reuses the machinery this PR already built.

Comment thread src/runtime/cli/mod.rs Outdated
robobun added 2 commits July 25, 2026 00:17
…-w/--prefix

- npm/npx links are only created when npm/npx are not already in the
  original PATH, so --bun never shadows a real npm. A stale link from
  a previous run is removed.
- npm exec -> bun x (bun's own exec is a shell-script runner)
- npm init <x> / npm create <x> -> bun create <x>; bare npm init
  stays bun init
- --workspace/-w -> --filter and --prefix/-C -> --cwd, hoisted after
  the mapped subcommand so bun run's stop-after-first-positional does
  not pass them through to the script
- new tests for each of the above; the two shim-dir tests are
  sequential because the shim dir is process-shared
Incorporate the per-subcommand keep list so --tag/--access/--otp reach
bun publish and --message reaches bun pm version instead of being
dropped. Map npm version to bun pm version.
@robobun

robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

The npx sibling path is covered as of 6ba698c: when invoked as npx, the same npm config flag list (plus -c/--call) is filtered from argv up to the first positional, so npx --loglevel error <pkg> no longer takes error for the package name. Args after the package name pass through untouched since they belong to the executed package. Also in that commit: --prefix is rewritten to --cwd and --pack-destination to --destination (npm pack now maps to bun pm pack) instead of being dropped.

@robobun
robobun force-pushed the farm/311a28c7/npm-npx-shim branch from 6ba698c to 9b0a189 Compare July 25, 2026 00:24
autofix-ci Bot and others added 2 commits July 25, 2026 00:26
…cement

npx accepts the same config flags as npm since npm 7, so drop the npm-only value-taking flags (plus -c/--call) appearing before the package name; args after the package name belong to the executed package. Map npm pack to bun pm pack with --pack-destination rewritten to --destination. Place hoisted flags between the first mapped token and the rest so npm test --prefix X becomes bun run --cwd X test instead of forwarding --cwd to the script, and drop hoisted flags for the bunx-mapped subcommands whose parser would take the stray value for the package name. Tests: async spawns so test.concurrent overlaps, positive assertion in the pnpm guard, no swallowed link errors, coverage for publish/version/pack/npx flag handling.
@robobun

robobun commented Jul 25, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:31 PM PT - Jul 24th, 2026

@robobun, your commit cef8bf7 is building: #79887

@robobun

robobun commented Jul 25, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 9:08 PM PT - Jul 25th, 2026

❌ @robobun, your commit 564ecc1 has 1 failures in Build #81384 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 35474

That installs a local version of the PR into your bun-35474 executable, so you can run:

bun-35474 --bun

Comment thread src/runtime/cli/mod.rs Outdated
Comment thread src/runtime/cli/mod.rs Outdated
Comment thread src/runtime/cli/mod.rs Outdated
… rename --save-* flags

npm upgrade (alias of update) dispatched bun's self-upgrader, replacing the binary; map upgrade/up/udpate to bun update. npm cache maps to bun pm cache and npm c (config alias) to bun config instead of bun create; npm v joins the view aliases. The subcommand is now found by a prescan before the first flag pass so the per-subcommand keep list covers flags in any position (npm --tag beta publish no longer drops the tag); kept flags hoist after the subcommand token since which()'s leading-flag skip would take their value for the subcommand. npm's boolean --save-dev/--save-exact/--save-optional/--save-peer rewrite in place to --dev/--exact/--optional/--peer so bun's parser does not silently skip them.
Comment thread src/install/lib.rs
Comment thread src/runtime/cli/mod.rs Outdated
robobun added 2 commits July 25, 2026 01:11
…te from translated args

The Windows shim branch skipped npm.exe/npx.exe when a real npm exists but left a stale hardlink from a previous run in the persistent shim dir, which is prepended to PATH, so it kept shadowing the real npm; delete it like the POSIX branch does. The init/create decision now reads the translated tail instead of raw argv, so a value-flag's value (npm init --loglevel error) no longer counts as an initializer.
DeleteFileW on a hardlink whose image is mapped by a running process fails with ERROR_ACCESS_DENIED, and the stale npm.exe/npx.exe links target the running bun binary, so the no-shadow cleanup silently did nothing and the shim kept shadowing a real npm. A mapped image can still be renamed, so fall back to renaming the link to <name>.exe.stale, which PATH resolution never matches. Verified the delete-denied/rename-allowed behavior against a running image on Windows.
Comment thread src/install/lib.rs
Comment thread src/runtime/cli/mod.rs
…by parser support

A nested --bun inherits PATH with the shim dir prepended, so the npm/npx probe found its own link, concluded a real npm exists, and deleted the only npm on PATH mid-script (release builds only; the debug wipe runs before the probe). Probe hits inside the shim dir no longer count as real, on both POSIX and Windows. The --workspace to --filter rename now applies only to subcommands whose bun parser accepts --filter (run scripts, install, update, outdated); elsewhere it is dropped so the workspace name cannot leak into positional position. Renamed flags are also no longer hoisted into bun create, whose scanner would take the value for the template name.
Comment thread src/install/lib.rs
Comment thread test/cli/run/as-npm.test.ts Outdated
Comment thread src/runtime/cli/mod.rs
Comment thread src/install/lib.rs Outdated
…rename by errno

npm still reads the token after a leading -- as the subcommand, so npm -- upgrade must map to bun update rather than dispatching the raw token (bun's self-upgrader). The Windows stale-link rename fallback now only runs when deletion failed with EPERM/EACCES/EBUSY (mapped image); a missing link needs no fallback. The cache test asserts the printed path exists.
Comment thread src/runtime/cli/mod.rs
Comment thread src/runtime/cli/mod.rs Outdated
Comment thread src/runtime/cli/mod.rs Outdated
Comment thread src/runtime/cli/mod.rs
… flag hygiene

npm's -O maps to --optional and -P/--save-prod map to the no-op --save since bun's own -P means --prod and would skip devDependencies. The registry, omit, ca, cafile, include, proxy, https-proxy, and noproxy value flags join the classifier so a pre-subcommand placement no longer eats the subcommand; the install family and publish keep the spellings bun accepts. The init/create template scan no longer stops at --, bun create's scanner no longer takes single-dash tokens for positionals, and the create mapping drops remaining npm config flags before -- since bun create rejects flags it does not know.
Comment thread src/install/lib.rs Outdated
Comment thread src/install/lib.rs Outdated
Comment thread src/install/lib.rs Outdated
Comment thread src/install/lib.rs Outdated
Comment thread src/install/lib.rs Outdated
Comment thread src/runtime/cli/mod.rs
Comment thread src/runtime/cli/mod.rs Outdated
Comment thread src/runtime/cli/mod.rs Outdated
Comment thread src/runtime/cli/mod.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 src/runtime/cli/mod.rs:1263-1289 — translate_npx_argv's loop breaks at the first non---prefixed token but doesn't recognize that npx's own -p/--package consumes a value, so spawnSync('npx', ['--package', 'typescript', '--loglevel', 'error', 'tsc']) breaks at typescript, hits the out.len() == i early return, and passes argv unchanged to bunx — which consumes --package typescript, silently skips --loglevel, and takes error as the binary name. Distinct from the open comment at line 871 (that's the npm subcommand prescan's missing NPM_VALUE_FLAGS_IGNORED entries; here the mechanism is npx's break-at-first-positional not accounting for -p/--package's value). Recognize -p/--package[=] in this loop and push both flag and value to out before advancing.

    Extended reasoning...

    What the bug is

    translate_npx_argv (mod.rs:1261-1284) strips npm config flags before the package name by breaking its loop at the first token whose first byte is not - — the doc comment says "everything from the package name on belongs to the executed package". But npx's own -p/--package flag takes a value that is a bare package name. translate_npm_value_flag returns None for --package (b"package" is not in NPM_VALUE_FLAGS_IGNORED — the sorted list goes …otp, pack-destination, script-shell… with no package), so --package falls through to out.push(a) and its bare value becomes the loop's break point. Any npm config value-flag placed after --package <pkg> is therefore never stripped and reaches bunx's parser, which consumes --package <pkg> correctly but then treats the config flag's stray value as the command name.

    Step-by-step proof

    For spawnSync('npx', ['--package', 'typescript', '--loglevel', 'error', 'tsc']) on a bun-only host:

    translate_npx_argv (mod.rs:1261-1284):

    1. i=1, ab=b"--package". Not -c/--call/--call=. translate_npm_value_flag(b"--package", &[], &[]) strips -- → name=b"package"; not in keep=[], not in renames=[], not b"prefix", NPM_VALUE_FLAGS_IGNORED.binary_search(&b"package") → Err → returns None. Falls through: out.push("--package"), i=2.
    2. i=2, ab=b"typescript". ab.first() != Some(&b'-') → break. out=[argv0, "--package"], out.len()==2==i → early return, argv unchanged.

    BunxCommand::Options::parse (bunx_command.rs:96-163) receives [npx, --package, typescript, --loglevel, error, tsc]:
    3. i=0 "npx" → found_subcommand_name=true.
    4. i=1 "--package" → i+=1, specified_package=Some("typescript"), then i+=1 → i=3.
    5. i=3 "--loglevel" → starts with -, matches no branch in the if/else chain (lines 106-153) → silently skipped, i=4.
    6. i=4 "error" → not --prefixed, found_subcommand_name already true → maybe_package_name=Some("error"), i=5.
    7. i=5 "tsc" → maybe_package_name.is_some() → passthrough_list.push("tsc").
    8. Line 166-189: specified_package=Some → opts.binary_name=Some("error"), opts.package_name="typescript".

    Result: bunx installs typescript and tries to run binary error with arg tsc. Real npx would run tsc from typescript. Same result for the -p short form.

    Why nothing prevents it

    b"package" is not in NPM_VALUE_FLAGS_IGNORED (nor should it be — bunx accepts --package natively, so dropping it would be wrong). The npx loop has no special case for -p/--package alongside its -c/--call special case, so the flag is pushed through and its value trips the break. The reverse ordering (npx --loglevel error --package typescript tsc) works correctly because --loglevel is stripped before --package is reached; only config flags positioned between -p <pkg> and the command hit this. The existing test at as-npm.test.ts:255 (npx --loglevel error --version) uses the working ordering.

    This is distinct from the open comment at line 871 on NPM_VALUE_FLAGS_IGNORED missing --omit/--include/--registry. That comment is about the npm subcommand prescan mis-locating the subcommand when an unlisted value-flag precedes it. This is the npx loop's break-at-first-positional not accounting for -p/--package consuming its value — a flag that bunx itself accepts and that must reach it, not be dropped.

    Impact and severity

    Nit. The shim is documented best-effort and only reached when no real npx is on PATH. Placing an npm config flag between --package <pkg> and the command name is an uncommon ordering — most tools put config flags first (where the loop handles them correctly) or use =. The pre-PR baseline was ENOENT on a bun-only host, so this is an incompleteness in a new best-effort feature, not a regression. But it is the direct sibling of the npm-side value-flag handling this PR carefully covers, and the failure mode (running the wrong binary) is confusing.

    How to fix

    Recognize -p/--package/--package=/-p= in translate_npx_argv's loop and push both the flag and its value to out before advancing, e.g. alongside the existing -c/--call check add a branch that does out.push(a); if consumes_next { out.push(argv[i+1]); i += 2 } else { i += 1 }; continue; for -p/--package. That keeps --package typescript in out (bunx wants it) while letting the loop continue past the value to strip the trailing --loglevel error.

translate_npx_argv broke at --package's bare value, so config flags after --package <pkg> reached bunx and their value was taken for the binary name. Recognize -p/--package as bunx's own value flag: keep it and its value and continue stripping.
@robobun

robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

The npx --package gap is fixed in 2105f8b: -p/--package (and the = form) are recognized as bunx-owned value flags, so the flag and its value pass through and config flags after them are still stripped. Test covers npx --package some-pkg --loglevel error, where bunx now asks for a binary name instead of taking error for it.

Comment thread src/runtime/cli/mod.rs
npm parses config flags position-independently, but bun run forwards everything after the script name to the script, so npm run go --silent passed --silent to the script and npm run missing --if-present exited 1. For the run mapping, flag tokens before -- now move out of passthrough position so bun's parser consumes them; args after -- still reach the script. Also split the overgrown init test so it stays under the debug-build timeout.
Comment thread src/runtime/cli/mod.rs Outdated
The run-family hoist moved a flag but left its value behind, so npm run dev --port 3000 became bun run --port dev 3000 and dispatched the wrong script. Match npm's parser instead: unknown config flags consume their following value and are dropped; only --if-present and --silent, which bun run parses, are hoisted.
Comment thread src/runtime/cli/mod.rs
Comment thread src/runtime/cli/mod.rs Outdated
The run and create sanitizers paired any unlisted flag with the next token, so npm run -s build swallowed the script name and npm init --scope @myorg took @myorg for a template. Boolean npm flags (including --no-* and the short loglevel aliases) now drop alone, scope joins the value-flag list, and the create sanitizer uses the same pairing rules as the run one.
Comment thread src/runtime/cli/mod.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/install/lib.rs`:
- Around line 752-767: Update the shim-directory exclusion in the real_in_path
closure to require the matched shim directory prefix to end at a path separator,
so sibling directories with the same textual prefix are not excluded. Preserve
the existing case-insensitive comparison and npm/npx detection behavior.
- Around line 800-819: Update the stale-shim rename in the MoveFileExW call
within the unlink error-handling block to pass a Win32 \\?\-prefixed path rather
than an NT \??\-prefixed path. Ensure both target_path_buffer and stale_buf use
the Win32 path format, or route the operation through an NT-aware helper, while
preserving the existing rename behavior.

In `@src/runtime/cli/mod.rs`:
- Around line 1283-1305: Update the argument-filtering loop around the package
and call checks to also detect npm-only boolean flags via is_npm_bool_flag and
skip them before out.push(a). Preserve existing handling for shared bunx flags
and value-taking npm options, and continue advancing i correctly without
consuming unrelated arguments.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6acef922-4d6e-4e09-b7da-67b7854821e1

📥 Commits

Reviewing files that changed from the base of the PR and between 27b2df9 and 466c7fa.

📒 Files selected for processing (5)
  • src/install/PackageManager.rs
  • src/install/lib.rs
  • src/runtime/cli/mod.rs
  • src/runtime/cli/run_command.rs
  • test/cli/run/as-npm.test.ts

Comment thread src/install/lib.rs
Comment thread src/install/lib.rs
Comment thread src/runtime/cli/mod.rs
Comment thread src/runtime/cli/mod.rs
Comment thread src/runtime/cli/mod.rs
Comment thread src/runtime/cli/mod.rs
Comment thread src/runtime/cli/run_command.rs
…dored .bin npm

is_npm_bool_flag gains the remaining npm boolean config flags (-f/--force, --dry-run, the offline and peer-deps toggles, --save, --audit, --fund, --package-lock-only, --production) so they drop alone instead of eating the next token. The run and init mappings now drain pre-subcommand flags (hoisting only --if-present/--silent), since npm shorts like -d collide with value-taking bun options and a leading flag broke init's positional offset. The npx filter drops npm-only booleans too. The shim probe also checks node_modules/.bin so a vendored npm is not shadowed, and both shim-dir comparisons anchor at a path separator.
Comment thread src/runtime/cli/mod.rs
Comment thread src/runtime/cli/mod.rs
…ding install shorts

npm -p exec foo fed -p to bunx, which took the mapped x token for the package name; the pre-subcommand drain now also covers the x and pm mappings. For the install-family subcommands, npm's -p/-y/-d (parseable, yes, loglevel shorthands) collide with bun's --production/--yarn/--dev and are dropped before and after the subcommand up to --. The multi-spawn dispatch tests run serially now; two dozen concurrent debug spawns kept tripping the per-test timeout under load.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4de95c8 addresses both open threads from the previous pass — the install-family -p/-y/-d short collisions are now dropped via drop_colliding_shorts (with the new "npm lowercase shorts" test), and pre_subcommand_flags is drained for the x/pm mappings (covered by the npm -p exec --help assertion). The remaining inline note is the tail-side sibling for npm exec and is narrow.

Extended reasoning...

The two unresolved inline comments I left at 14:53Z (install-family -p/-y/-d colliding with bun's --production/--yarn/--dev, and the x/pm mappings not draining pre_subcommand_flags) are both fixed in the latest commit 4de95c8, each with a test in as-npm.test.ts. I re-traced both shapes through the current diff and they now produce the intended argv. The one new finding this run is a nit-level completeness gap on the same sanitizer class (tail side of npm exec), not a regression. This PR remains large enough — ~540 lines of new argv-rewriting logic in the CLI entry point plus Windows shim-dir FFI — that it warrants a human sign-off, so not approving.

Comment thread src/runtime/cli/mod.rs
npm exec -p foo fed -p to bunx as --package, which consumed foo for the package name; npm's own -p is the --parseable boolean. The x mapping now filters its tail like the npx path: npm booleans drop (keeping --silent, which bunx parses) and -c/--call drops with its value.
Comment thread src/runtime/cli/mod.rs
The install-family short filter also drops -dd/-ddd, which bun's parser would chain as -d -d (--dev). -l joins the boolean list as --long's short. -m joins -w/-C as npm's third value-taking short, mapping to message, so npm -m msg version patch no longer takes the message for the subcommand.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bugs found, but this adds a substantial new compatibility surface — ~570 lines of npm→bun argv translation with many hand-maintained flag allowlists, plus a change to the --bun shim-dir semantics — so it warrants a human sign-off on the design.

What was reviewed:

  • The npm/npx argv translators (subcommand mapping, value-flag/boolean-flag pairing, pre-subcommand drain, per-mapping tail sanitizers) — the remaining shorthand collisions raised in earlier rounds are closed.
  • create_fake_temporary_node_executable on both POSIX and Windows — the shim-dir self-exclusion, the node_modules/.bin probe, and the Windows stale-link rename fallback.
  • Ruled out: npm uninstall -w losing the workspace filter — -w is translated to --filter and hoisted, and remove accepts it.
Extended reasoning...

Overview

The PR extends the --bun fake-node shim directory to also plant npm/npx links (only when no real npm is resolvable in PATH or node_modules/.bin), and teaches the CLI entrypoint to recognize argv0 == npm/npx and rewrite argv into an equivalent bun invocation. Files touched: src/runtime/cli/mod.rs (+~570 lines: translate_npm_argv, translate_npx_argv, flag tables and helpers; which() and the pre-which fast-path guard), src/install/lib.rs (npm/npx link creation, Windows stale-link removal via MoveFileExW), src/runtime/cli/run_command.rs and src/install/PackageManager.rs (thread original_path through), and a new 530-line test file.

Security risks

Low. The shim links are created in the same bun-node-* temp dir that already hosts the node/bun shims, under the same 0700/owner check. The argv translation is pure in-process rewriting of the process-global argv view during single-threaded startup; no shell interpolation or external input beyond what bun already parses. The one nuance — silently taking over npm on a bun-only host — is an intentional product decision, not a security defect.

Level of scrutiny

High. This is not a mechanical fix: it introduces a new user-facing compatibility surface (bun-as-npm) with a large hand-maintained mapping of npm subcommands, aliases, and config flags to bun equivalents. The PR went through nine fix commits, each closing another sibling gap in the flag allowlists (-P/-O, then -s/-q, then --force/--dry-run, then pre-subcommand drain, then -p/-y/-d for install, then the x/pm tail, then -dd/-l/-m). That pattern — a heuristic that keeps sprouting counterexamples — is exactly what REVIEW.md flags for maintainer attention on the structural approach. The author also explicitly scoped down the .bin probe to cwd-only, deferring the ancestor walk as follow-up.

Other factors

Test coverage is thorough for the cases enumerated during review, and all prior inline threads are resolved. But the core design choices — whether to shim npm at all under --bun, whether the argv-rewrite approach is preferable to a dedicated npm-mode dispatcher, and where to draw the line on flag-table completeness — are product/architecture calls a maintainer should own.

@robobun

robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for maintainer review. All review rounds are addressed and every thread is resolved; the test file covers shim creation and precedence (env PATH, vendored node_modules/.bin, nested --bun), argv0 dispatch for npm/npx, and the flag translation table (drop/keep/rename/hoist, value and boolean shorts, -- handling). The latest CI run is green on all test lanes; the only red is a pre-existing build-infra step on the linux x64-android lane that also fails on main.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bun create react-app fails unless Node is installed create react-app fails if npm is not in path

1 participant