Skip to content

shell: resolve commands on the shell environment's PATH, not the startup snapshot, and scope VAR=value prefixes to one command - #42049

Open
robobun wants to merge 9 commits into
mainfrom
robobun/717bc61e/shell-no-launch-path-fallback
Open

robobun wants to merge 9 commits into
mainfrom
robobun/717bc61e/shell-no-launch-path-fallback

Conversation

@robobun

@robobun robobun commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • $`tool` runs a tool found only on the PATH Bun started with, after delete process.env.PATH or with an .env({...}) object without PATH. node:child_process, Bun.spawn({ env }) and the shell's which say not found. On Windows (key Path), export PATH=... and runtime process.env.PATH edits never reached the lookup.
  • Cause: SpawnArgs::default seeded the lookup PATH from the startup environment snapshot (src/runtime/shell/subproc.rs), and fill_env overrode it only for a key spelled exactly PATH. cmd_local_env was also never cleared, so a VAR=value cmd prefix reached every later command.

Fix

  • One lookup, ShellExecEnv::command_path(): a PATH=... cmd prefix, then the exported environment (case-insensitive on Windows), else _PATH_DEFPATH on POSIX and the process's current PATH on Windows. Cmd.rs and which call it. A command clears cmd_local_env when it starts.
  • Correct because Bun.spawn({ env }) (Fix bug with PATH in Bun.spawn #16067), node:child_process and libuv apply this rule to an env without PATH, and bash scopes a prefix assignment to its command. PATH="" still searches nothing.
  • Behavior change on POSIX: $.env({ FOO }) plus a command outside /usr/bin:/bin now fails with bun: command not found. Docs and types now say .env() replaces the environment. The export var test expected the prefix leak and is updated.
  • Replaces shell: resolve commands and which through one PATH lookup on the shell environment #40565 (same refactor, kept the snapshot fallback, now closed). Its Windows-only tests are carried over here. Self-reviewed, see Notes. Verified: the new shell tests fail on 1.4.3 and pass here, Linux x64 and Windows x64.

Background

  • Each $`...` builds a shell whose export_env copies process.env at call time, or the .env() object. cmd_local_env holds VAR=value cmd prefixes.
  • _PATH_DEFPATH (/usr/bin:/bin) is what execvp searches when PATH is unset. libuv on Windows searches the parent's current PATH instead.
Notes

Repro on Linux with 1.4.3-canary (d is a temp dir with an executable w5891hi, PATH=$d:$PATH bun repro.mjs):

after `delete process.env.PATH`:
  child_process.spawnSync("w5891hi")               -> ENOENT
  Bun.spawnSync(["w5891hi"], { env: process.env }) -> throws ENOENT
  $`w5891hi`                                       -> RAN (exit 0)   <- the bug
  $`which w5891hi`                                 -> which: w5891hi not found
with `process.env.PATH = ""` the shell already reported `command not found`; only an absent key fell back.

Real node 26 for comparison: an env without PATH finds /usr/bin tools and not the temp dir tool; PATH="" finds nothing. bash and dash set a default PATH when started without one.

Windows x64 probe (canary b52d3e5 vs this branch), process.env exposes the key as Path:

                                          canary               this branch
.env({ ...env, Path: dir;... })   tool    command not found    runs
export PATH=dir;... (env has Path) tool   command not found    runs
process.env.PATH = dir;...  then  tool    command not found    runs
.env(envWithoutPath)   launch-PATH tool   runs                 runs (current process PATH)
delete process.env.PATH;  launch tool     runs                 command not found
PATH=dir cmd prefix, .env({ PATH })       runs                 runs

On Windows process.env writes go through SetEnvironmentVariableW, so the current process PATH reflects runtime edits and deletes. On POSIX they do not reach the native environment (#40846 tracks Bun.which / Bun.spawn reading the startup snapshot), one more reason the POSIX arm does not consult the process environment at all.

which changes with it: PATH=<dir> which tool now searches <dir> (bash does the same), and which in an environment without PATH uses the same default as running the command.

Prefix scope: FOO=leak true; printenv FOO printed leak and PATH=/nonexistent true; sh -c 'echo hi' failed with command not found: sh on 1.4.3, because cmd_local_env kept the prefix for the rest of the script (the Zig implementation did the same). bash, dash and POSIX scope the assignment to the one command. Cmd now clears cmd_local_env when it starts, which covers the child environment, the argv[0] lookup and which. The bunshell > variables > export var test asserted that a later command still saw BAZ=1 from an earlier prefix; its expectation is updated with a comment. The never-instantiated DISABLE_PATH_LOOKUP_FOR_ARV0 generic on fill_env and the dangling // PATH = ""; note in ParsedShellScript.rs go away with SpawnArgs.path.

Not addressed here, tracked separately: #32202 (a VAR=value cmd prefix for a variable that is also exported produces two entries in the child's environment block).

Self-review asks that this revision addresses: carry a PATH=<dir> which tool test (commands/which.test.ts), keep a prefix assignment from reaching later commands now that which reads cmd_local_env, stop the docs examples from modelling the minimal-object pattern right below the new note, use the process's current PATH on Windows instead of searching nothing, and state the relation to #40565 here and on that PR.

Tests carried over from #40565 (Windows-only, in the external command resolution on Windows block): .env() with PATH added to an object that already has Path (the { ...process.env, PATH } pattern), a PATH= prefix over export PATH (extended to check that the next command uses the exported PATH again), and export PATH in a package.json script run by bun run. All three fail with bun: command not found on 1.4.3-canary (d745f03) and pass with this branch on Windows x64. The which falls back to the process PATH test from #40565 is not carried over: it asserts the startup-snapshot fallback that this PR removes on POSIX, and external command resolution without a PATH ignores the launch PATH covers the new rule.

Suites run with the debug build. Linux x64: test/js/bun/shell/bunshell.test.ts, test/js/bun/shell/commands/, bunshell-instance, bunshell-default, exec, test/js/bun/spawn/spawn-path.test.ts, test/cli/install/bun-run.test.ts (the only failures are two ls permission tests that fail as root on main too). Windows x64: bunshell.test.ts, commands/which.test.ts, commands/, bunshell-instance, bunshell-file, exec, test/cli/install/bun-run.test.ts (pre-existing: five tilde_expansion tests that need HOME, one rm test that trips on a debug-only warning).


no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/commands/which.test.ts, test/js/bun/shell/bunshell.test.ts

…ronment has none

The lookup PATH for argv[0] now comes only from the shell environment:
a PATH=... prefix assignment, then the exported environment, then the
platform default for an environment without PATH (_PATH_DEFPATH on
POSIX). The which builtin resolves through the same helper.
…l environment has none

Matches node:child_process and libuv on Windows, which search the current
process PATH and copy it into a child whose environment block lacks one.
Document that .env() replaces the whole environment.
cmd_local_env was never cleared, so a prefix assignment reached the
environment and the PATH lookup of every later command in the same
shell environment. Clear it when a command starts.
@robobun
robobun requested a review from alii as a code owner September 8, 2026 17:47
@coderabbitai

coderabbitai Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file.

Or wait 30 seconds for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: e5c94043-67e4-4f46-b609-ad27de7baee6

📥 Commits

Reviewing files that changed from the base of the PR and between 85e9ddc and 1b1f6b8.

📒 Files selected for processing (10)
  • docs/runtime/shell.mdx
  • packages/bun-types/shell.d.ts
  • src/runtime/shell/EnvMap.rs
  • src/runtime/shell/ParsedShellScript.rs
  • src/runtime/shell/builtin/which.rs
  • src/runtime/shell/interpreter.rs
  • src/runtime/shell/states/Cmd.rs
  • src/runtime/shell/subproc.rs
  • test/js/bun/shell/bunshell.test.ts
  • test/js/bun/shell/commands/which.test.ts

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

@github-actions github-actions Bot added the claude label Sep 8, 2026
@robobun

robobun commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:42 PM PT - Sep 8th, 2026

✅ @robobun, your commit 1b1f6b8beb8265a8a3b3cb8c6d98a008aa1471dc passed in Build #113268! 🎉


🧪   To try this PR locally:

bunx bun-pr 42049

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

bun-42049 --bun

@robobun

robobun commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

  • Reproduced on Linux x64 with 1.4.3-canary (f42e980): with a tool that exists only in a directory on the launch PATH, delete process.env.PATH followed by $`tool` still ran it, while child_process.spawnSync("tool") gave ENOENT, Bun.spawnSync(["tool"], { env: process.env }) threw ENOENT, and $`which tool` printed not found. $`tool`.env({}) ran it too.
  • Reproduced the Windows side on windows-x64 with the canary (b52d3e5): .env({ ...env, Path: dir }), export PATH=... over an environment with Path, and a runtime process.env.PATH edit all gave bun: command not found, and a tool on the launch PATH still ran after delete process.env.PATH.
  • Fail before / pass after: test/js/bun/shell/bunshell.test.ts ("external command resolution without a PATH ignores the launch PATH", "a VAR=value prefix applies to its own command only", and the "external command resolution on Windows" group) and test/js/bun/shell/commands/which.test.ts ("which honors a PATH= prefix assignment") fail on 1.4.3 and pass with this branch, checked on Linux x64 and Windows x64.
  • CI (build 113268 on 1b1f6b8): every lane that has run passes, with only retried flakes unrelated to the shell; the two darwin x64 test jobs are still queued. Ready for review.

PR: #42049

Comment thread src/runtime/shell/states/Cmd.rs Outdated
Comment thread src/runtime/shell/interpreter.rs Outdated
Comment thread src/runtime/shell/states/Cmd.rs Outdated
Comment thread src/runtime/shell/subproc.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.

Code review found no issues

No high-confidence issues detected in this change.

Comment thread src/runtime/shell/interpreter.rs
Comment thread src/runtime/shell/subproc.rs

@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.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

Note for a rebase: #43055 moves the which builtin's not-found line from stdout to stderr (which: <arg> not found). If it lands first, two assertions in this PR need the stderr form: the "deleted PATH, which" row in test/js/bun/shell/bunshell.test.ts (stdout becomes "", stderr becomes "which: onlyinlaunchpath not found") and the which: tool not found expectation in test/js/bun/shell/commands/which.test.ts. The PATH lookup change here is not affected.

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.

1 participant