Skip to content

Bun.spawn/spawnSync, Bun.which: inherit live process.env instead of the startup snapshot - #34972

Open
robobun wants to merge 7 commits into
mainfrom
claude/farm/68b2c3e5/spawn-default-env-live-process-env
Open

Bun.spawn/spawnSync, Bun.which: inherit live process.env instead of the startup snapshot#34972
robobun wants to merge 7 commits into
mainfrom
claude/farm/68b2c3e5/spawn-default-env-live-process-env

Conversation

@robobun

@robobun robobun commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Repro

process.env.MYVAR = "hello";
delete process.env.DELME;                    // DELME was set at process start
console.log(String(Bun.spawnSync({
  cmd: ["/bin/sh", "-c", "echo MYVAR=[$MYVAR] DELME=[$DELME]"],
}).stdout));
// before: MYVAR=[] DELME=[startup-value]   (set invisible, delete invisible)
// after:  MYVAR=[hello] DELME=[]
process.env.PATH = dir + ":" + process.env.PATH;
Bun.which("tool");            // before: null    after: /.../tool
Bun.spawnSync(["tool"]);      // before: ENOENT  after: runs

child_process.spawn* was already correct (it always passes an explicit env built from process.env on the JS side). env: {...process.env} was already correct.

Cause

  • js_bun_spawn_bindings.rs: the no-env: fallback serialized jsc_vm.transpiler.env_mut().map (the bun_dotenv::Loader map, populated once from libc environ at startup), and the initial PATH for argv0 lookup came from jsc_vm.env_loader().get(b"PATH"), the same snapshot.
  • BunObject.rs::which: default PATH came from vm.env_loader().get(b"PATH").
  • process.env.X = ... (the generic jsSetterEnvironmentVariable in JSEnvironmentVariableMap.cpp) writes only to the JS object, never the native map; delete process.env.X likewise.

Fix

Expose ZigGlobalObject::processEnvObject() to Rust and use the live process.env JS object as the default env source:

  • Bun.spawn/spawnSync with no env: now enumerates processEnvObject() via the existing append_envp_from_js, which also extracts PATH for argv0 lookup. This is the same code path env: {...process.env} already exercised.
  • Bun.which now reads its default PATH from processEnvObject().PATH.

Verification

$ USE_SYSTEM_BUN=1 bun test test/js/bun/spawn/spawn-env.test.ts -t "default env"
(fail) Bun.spawnSync({cmd})  Expected "runtime-value,[unset]" Received "[unset],startup-value"
(fail) Bun.spawnSync([cmd])  Expected "runtime-value,[unset]" Received "[unset],startup-value"
(fail) Bun.spawn({cmd})      Expected "runtime-value,[unset]" Received "[unset],startup-value"
(fail) PATH mutation ...     ENOENT: Executable not found in $PATH: "bun_test_spawn_env_tool"

$ bun bd test test/js/bun/spawn/spawn-env.test.ts
5 pass, 0 fail

test/js/bun/spawn/spawn.test.ts, test/js/bun/spawn/spawn-path.test.ts, test/js/bun/util/which.test.ts, test/js/bun/spawn/null-byte-injection.test.ts all pass.


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/spawn/spawn-env.test.ts

…he startup snapshot

When no env option is passed, Bun.spawn/Bun.spawnSync serialized the native
DotEnv loader map (populated once from libc environ at startup), and both the
default envp and the PATH used for argv0 lookup ignored any
process.env.X = '...' or delete process.env.X done at runtime. Bun.which read
PATH from the same stale map.

Now the default environment is built by enumerating globalObject->processEnvObject()
through the same append_envp_from_js path that an explicit env: {...process.env}
already took, and Bun.which reads PATH from the same object. Runtime mutations
to process.env, including prepending a directory to PATH, are now visible to the
child and to argv0/which lookup.
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Live process.env integration

Layer / File(s) Summary
process.env FFI bridge
src/jsc/JSGlobalObject.rs, src/jsc/bindings/ZigGlobalObject.cpp
Adds Rust and C++ bindings that return the live process.env object and propagate JavaScript exceptions.
PATH and spawn environment resolution
src/runtime/api/BunObject.rs, src/runtime/api/bun/js_bun_spawn_bindings.rs, test/js/bun/spawn/spawn-env.test.ts
Updates Bun.which and default child-process environment construction to read live process.env, with tests for runtime environment and PATH mutations.

Possibly related PRs

  • oven-sh/bun#34290: Changes child-process environment population in the same spawn binding.
  • oven-sh/bun#34728: Changes string coercion for values assigned to the POSIX process.env backing.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: live process.env inheritance for Bun.spawn/spawnSync and Bun.which.
Description check ✅ Passed It covers the change, root cause, fix, and verification, so it satisfies the template's intent despite using different headings.

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

@robobun

robobun commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:04 PM PT - Jul 21st, 2026

@robobun, your commit 3819377efbed49c812ab0f5e291224e16a11619e passed in Build #77046! 🎉


🧪   To try this PR locally:

bunx bun-pr 34972

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

bun-34972 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 2 issues this PR may fix:

  1. child_process.execFileSync uses stale PATH for command resolution when options.env is omitted #29237 - Reports that child_process.execFileSync uses the startup PATH snapshot for command resolution when options.env is omitted, ignoring runtime process.env.PATH mutations
  2. process.env.PATH issues #11182 - Reports that runtime modifications to process.env.PATH are not reflected in child_process.spawn command resolution

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #29237
Fixes #11182

🤖 Generated with Claude Code

@robobun

robobun commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator Author

Regarding the linked issues: #29237 and #11182 both go through child_process, which already spreads process.env into an explicit env: on the JS side, so their repros pass on current canary independently of this change. This PR fixes the underlying native default (what Bun.spawn/Bun.spawnSync/Bun.which do when no env: is passed), so any path that reaches the native fallback now behaves correctly too.

#10865 and #25885 are about Bun.$ shell command resolution, which has its own lookup path and is not touched here.

createEnvironmentVariablesMap declares a THROW_SCOPE, so the first call to
processEnvObject() (which forces the lazy init) must be followed by an
exception check before the next throw scope. Wrap the extern in its own
THROW_SCOPE with RETURN_IF_EXCEPTION and route the Rust side through
from_js_host_call so zero maps to Err.
Comment thread src/jsc/JSGlobalObject.rs
Comment thread test/js/bun/spawn/spawn-env.test.ts Outdated
Comment thread src/runtime/api/bun/js_bun_spawn_bindings.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: 1

🤖 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 `@test/js/bun/spawn/spawn-env.test.ts`:
- Line 19: Update the affected Bun child tests in the concurrent test blocks to
use sequential test registration: replace test.concurrent with test, and use
test.skipIf where conditional skipping is needed. Preserve each test’s existing
labels, bodies, and skip conditions while ensuring all four spawn cases run
sequentially.
🪄 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: 1265ce47-a016-4cc9-be56-1219fa1cad20

📥 Commits

Reviewing files that changed from the base of the PR and between e550f2c and b7a1626.

📒 Files selected for processing (5)
  • src/jsc/JSGlobalObject.rs
  • src/jsc/bindings/ZigGlobalObject.cpp
  • src/runtime/api/BunObject.rs
  • src/runtime/api/bun/js_bun_spawn_bindings.rs
  • test/js/bun/spawn/spawn-env.test.ts

Comment thread test/js/bun/spawn/spawn-env.test.ts
Comment thread test/js/bun/spawn/spawn-env.test.ts
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.

2 participants