child_process: honor runtime process.env.PATH in sync spawn variants#29239
child_process: honor runtime process.env.PATH in sync spawn variants#29239
Conversation
|
Updated 3:34 AM PT - Apr 13th, 2026
❌ @autofix-ci[bot], your commit d97d351 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29239That installs a local version of the PR into your bun-29239 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSpawn-sync now forwards the module-normalized environment slot ( Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29237.test.ts`:
- Around line 59-66: Replace the single combined assertion that compares {
stdout, exitCode } with two separate assertions: first assert stdout equals the
expected multi-line string (the block that currently appears under stdout in the
diff: "execFileSync: FAKE_CALLED\nspawnSync: FAKE_CALLED\nexecSync:
FAKE_CALLED\nexecFileSync+env: FAKE_CALLED\n"), then assert exitCode equals 0;
update the test that currently calls expect({ stdout, exitCode }).toEqual(...)
to use expect(stdout).toBe(...) followed by expect(exitCode).toBe(0) so failures
show stdout before exit-code.
🪄 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: 3e22a410-e77b-4bf7-90ef-4a545a1f1b28
📒 Files selected for processing (2)
src/js/node/child_process.tstest/regression/issue/29237.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/regression/issue/29237.test.ts (1)
53-60:⚠️ Potential issue | 🟡 MinorSplit this into ordered
stdoutthenexitCodeassertions.The combined object assertion makes failures less actionable and violates the repo’s assertion-order rule.
Suggested change
- expect({ stdout, exitCode }).toEqual({ - stdout: - "execFileSync: FAKE_CALLED\n" + - "spawnSync: FAKE_CALLED\n" + - "execSync: FAKE_CALLED\n" + - "execFileSync+env: FAKE_CALLED\n", - exitCode: 0, - }); + expect(stdout).toBe( + "execFileSync: FAKE_CALLED\n" + + "spawnSync: FAKE_CALLED\n" + + "execSync: FAKE_CALLED\n" + + "execFileSync+env: FAKE_CALLED\n", + ); + expect(exitCode).toBe(0);As per coding guidelines: "Expect stdout assertions before exit code assertions - expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0) for more useful error messages".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29237.test.ts` around lines 53 - 60, The current combined assertion expect({ stdout, exitCode }).toEqual(...) should be split into two ordered assertions: first assert stdout string content with expect(stdout).toBe("execFileSync: FAKE_CALLED\nspawnSync: FAKE_CALLED\nexecSync: FAKE_CALLED\nexecFileSync+env: FAKE_CALLED\n"), then assert the exit code with expect(exitCode).toBe(0); update the test in issue/29237.test.ts to replace the single expect(...) call with these two ordered assertions so failures report stdout mismatches before exitCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29237.test.ts`:
- Around line 61-62: Remove the flaky generic stderr assertion by deleting or
replacing the expect(stderr).not.toContain("error:") check in the test (the line
that calls expect(stderr).not.toContain("error:") in issue/29237.test.ts); rely
on the existing exact stdout and exitCode assertions instead so the test no
longer fails due to unrelated environment/tooling messages.
---
Duplicate comments:
In `@test/regression/issue/29237.test.ts`:
- Around line 53-60: The current combined assertion expect({ stdout, exitCode
}).toEqual(...) should be split into two ordered assertions: first assert stdout
string content with expect(stdout).toBe("execFileSync: FAKE_CALLED\nspawnSync:
FAKE_CALLED\nexecSync: FAKE_CALLED\nexecFileSync+env: FAKE_CALLED\n"), then
assert the exit code with expect(exitCode).toBe(0); update the test in
issue/29237.test.ts to replace the single expect(...) call with these two
ordered assertions so failures report stdout mismatches before exitCode.
🪄 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: f42f47ee-dfd5-428d-a990-1622ea182f79
📒 Files selected for processing (1)
test/regression/issue/29237.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the one-line fix correctly routes spawnSync through options[kBunEnv] to match the already-correct async path.
Extended reasoning...
Overview
The PR makes a single-line change to spawnSync in src/js/node/child_process.ts, swapping options.env for options[kBunEnv]. A new regression test file is added at test/regression/issue/29237.test.ts.
Security risks
None. The fix does not change what environment is passed to child processes in any security-sensitive way — it simply ensures the live process.env (already captured by normalizeSpawnArguments) is used instead of falling back to Bun's startup-time snapshot.
Level of scrutiny
Low. This is a one-line bug fix that aligns spawnSync with the async ChildProcess.spawn path, which already used options[kBunEnv] correctly. The root cause analysis in the PR description is accurate and the fix is the minimal correct change.
Other factors
The two inline bug reports are both nits (test assertion style, dynamic require in test body) with zero functional impact. The fix itself is correct and well-tested. No CODEOWNERS files appear to govern src/js/node/child_process.ts changes of this scope.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/regression/issue/29237.test.ts:57-65— The test asserts stdout and exitCode together in a singleexpect({ stdout, exitCode }).toEqual({...})call, which slightly obscures whether stdout or exit code is wrong when the test fails. Splitting them intoexpect(stdout).toEqual(...)first andexpect(exitCode).toBe(0)last, as shown in the test/CLAUDE.md examples, gives a cleaner failure message.Extended reasoning...
What the bug is and how it manifests
The regression test at lines 57–65 of
test/regression/issue/29237.test.tsuses a combined object assertion:expect({ stdout, exitCode }).toEqual({ stdout: "execFileSync: FAKE_CALLED\n" + ..., exitCode: 0, });
When this assertion fails, the test runner shows a diff containing both
stdoutandexitCodeside-by-side. If only stdout is wrong the diff still shows exitCode as "received: 1" alongside it (or vice versa), making it harder to read at a glance which field is actually broken.The specific code path
Lines 57–65 of the new test file collect
stdout,stderr, andexitCodeviaPromise.all, then group two of them (stdoutandexitCode) into a single object assertion rather than checking each separately.Why existing guidance doesn't prevent it
The refutation correctly notes that the exact quoted phrase ("When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0)") does not appear verbatim in
test/CLAUDE.md. That is accurate — the rule is not spelled out as a named directive.However, both example test patterns in
test/CLAUDE.md(the single-file example at lines 48–50 and the multi-file example at lines 93–95) consistently use separate, ordered assertions:expect(stdout).toBe(...)beforeexpect(exitCode).toBe(0). The combined.toEqualform diverges from every template shown in that guide.The refutation also cites the "Nested/complex object equality" section (line 158), which says to prefer
.toEqualover many.toBecalls for nested or complex objects. A two-field flat object{ stdout, exitCode }is not what that guidance targets — it is aimed at multi-level object trees or arrays of records, where individual.toBechains become unwieldy.Impact
Purely a developer-experience issue. When a CI run shows this test failing, the error diff is noisier than it needs to be. No functional correctness is affected.
How to fix
expect(stdout).toEqual( "execFileSync: FAKE_CALLED\n" + "spawnSync: FAKE_CALLED\n" + "execSync: FAKE_CALLED\n" + "execFileSync+env: FAKE_CALLED\n", ); expect(stderr).not.toContain("error:"); expect(exitCode).toBe(0);
Step-by-step proof
- Suppose the fix in
child_process.tspartially works:execFileSyncandexecSyncnow produceFAKE_CALLEDbutspawnSyncstill uses the stale PATH and returns exit code 1. - With the current combined assertion the test runner prints something like:
Expected: { stdout: "...FAKE_CALLED\nFAKE_CALLED\n...", exitCode: 0 } / Received: { stdout: "...real-ls output\n...", exitCode: 1 }. Both fields appear in the same diff block, and the reader must mentally parse which one is actually wrong. - With separate assertions the first failure is
expect(stdout).toEqual(...), which immediately isolates the stdout content as the problem — the exit code assertion is never reached, so it produces no noise.
- Suppose the fix in
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29237.test.ts`:
- Around line 40-41: Replace the runtime require calls with top-level ES module
imports: remove the inline const { chmodSync } = require("node:fs") and const
path = require("node:path") and add module-scope imports like import { chmodSync
} from "node:fs" and import path from "node:path" at the top of the test file so
the symbols chmodSync and path are imported statically for use in the test
(refer to the existing uses of chmodSync and path in the file to confirm correct
import names).
🪄 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: b1342d1e-4414-419f-88de-c9137b64ad42
📒 Files selected for processing (1)
test/regression/issue/29237.test.ts
There was a problem hiding this comment.
LGTM — all prior review comments addressed, one-line fix is correct and well-tested.
Extended reasoning...
Overview
The PR makes a single-line change in src/js/node/child_process.ts: in spawnSync, it switches from options.env to options[kBunEnv] when calling Bun.spawnSync. This aligns the sync path with the async ChildProcess.spawn path, which already used options[kBunEnv]. The kBunEnv slot is populated by normalizeSpawnArguments from the live process.env at call time, so runtime mutations to process.env.PATH are now honoured. A new regression test covers execFileSync, spawnSync, and execSync.
Security risks
None. The change only affects which environment object is forwarded to the child process — it uses the same data source (process.env captured at spawn time) that the async path already used. No new attack surface is introduced.
Level of scrutiny
Low. The fix is mechanical: one symbol reference replacing another. The async path was already correct; the sync path was inconsistently reading options.env instead of the normalized copy. The change is trivially verifiable by reading the surrounding code.
Other factors
All prior reviewer comments (assertion ordering, flaky stderr check, dynamic-require convention) have been resolved in follow-up commits. The flagged parseEnvPairs bug is pre-existing and untouched by this PR. No CODEOWNER-sensitive paths are modified.
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed.
Extended reasoning...
Overview
One-line fix in src/js/node/child_process.ts changing spawnSync to pass options[kBunEnv] instead of options.env to Bun.spawnSync, mirroring the pattern already used in the async ChildProcess.spawn path. A regression test was added for the three affected APIs.
Security risks
None. The change reads from a symbol-keyed property set by normalizeSpawnArguments in the same file — no new attack surface.
Level of scrutiny
Low. The fix is mechanical and clearly correct: both paths should use the same normalized env object, and the async path already did this. The diff is a one-word change.
Other factors
All review feedback (assertion ordering, dropped flaky stderr check, and hoisting dynamic require() calls to top-level imports) was addressed in subsequent commits. The CI failures visible in the timeline are pre-existing infrastructure issues unrelated to this change.
There was a problem hiding this comment.
LGTM — all review feedback addressed, the one-liner fix is correct, and the Windows PATH normalization is a sound follow-on.
Extended reasoning...
Overview
This PR fixes a one-line bug in spawnSync (src/js/node/child_process.ts:537): the sync path was reading options.env instead of options[kBunEnv], causing runtime mutations to process.env.PATH to be ignored when spawning synchronously. A follow-up commit also normalizes the Windows PATH key casing in normalizeSpawnArguments so Bun.spawnSync's case-sensitive PATH lookup works correctly on Windows.
Security risks
None. The change only affects which env object is forwarded to Bun.spawnSync — the same object that was already being correctly used on the async path. No new attack surface is introduced.
Level of scrutiny
Low. This is a narrow, mechanical fix to an obvious inconsistency between the async and sync code paths. The root cause analysis in the PR description is accurate, the diff is minimal, and the regression test covers execFileSync, spawnSync, and execSync both with and without explicit env.
Other factors
All reviewer feedback (dynamic require hoisting, assertion ordering, Windows PATH normalization) has been addressed in subsequent commits. The pre-existing parseEnvPairs split-limit issue I noted is unrelated to this PR and does not affect the fix being reviewed.
There was a problem hiding this comment.
LGTM — all feedback addressed, one-line sync path fix is correct.
Extended reasoning...
Overview
The PR makes two changes to src/js/node/child_process.ts: (1) routes spawnSync through options[kBunEnv] instead of options.env so it uses the normalized env snapshot that normalizeSpawnArguments already captures from the live process.env; (2) canonicalizes any case-variant of the PATH key to "PATH" on Windows during env normalization. A regression test exercises all three sync variants.
Security risks
None. The change ensures the same environment object that was already being read for the async spawn path is forwarded to the sync path — no new attack surface.
Level of scrutiny
Low. This is a single-symbol swap fixing a clear asymmetry between the async and sync code paths. The logic was already correct in the async path; the sync path simply wasn't using the symbol-keyed property that normalizeSpawnArguments prepared for it.
Other factors
All reviewer comments (import hoisting, assertion ordering, flaky stderr check) were addressed in follow-up commits. No bugs introduced by this PR were found. The pre-existing parseEnvPairs split-limit issue I previously flagged is out of scope here.
spawnSync (and execFileSync / execSync which route through it) was reading options.env when forwarding to Bun.spawnSync. When the caller omits env, that value is undefined — so Bun.spawnSync fell back to its process-start environment snapshot and resolved bare command names against a stale PATH, ignoring runtime mutations to process.env.PATH. The async path (ChildProcess.spawn) already reads the freshly-copied options[kBunEnv] that normalizeSpawnArguments attaches. Route spawnSync through the same symbol so both sync and async honor runtime env mutations. Fixes #29237
Bun.spawnSync's argv[0] PATH lookup does a case-sensitive match on "PATH" (src/bun.js/api/bun/js_bun_spawn_bindings.zig:1096). On Windows, process.env iterates keys with their OS casing (e.g. "Path"), so the sync path after PR 29239 was copying bunEnv["Path"]=... and Bun.spawnSync couldn't find PATH for command resolution, breaking test-child-process-execsync.js on windows-2019 and windows-11-aarch64. Rename the PATH entry to uppercase when copying into bunEnv. Windows child processes treat env keys case-insensitively so this is transparent to them.
robobun
force-pushed
the
farm/b23657e5/fix-execfilesync-stale-path
branch
from
f27ac67 to
2063f63
Compare
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed and the core fix is correct.
Extended reasoning...
Overview
The PR fixes two things in src/js/node/child_process.ts: (1) a one-line bug where spawnSync was passing options.env to Bun.spawnSync instead of the normalized options[kBunEnv] snapshot built by normalizeSpawnArguments, and (2) a Windows-specific PATH key canonicalization in that same normalization function. A regression test is added at test/regression/issue/29237.test.ts.
Security risks
None. The change only affects which pre-built environment object is forwarded to the child process — the same data that was already assembled by normalizeSpawnArguments. No new privilege, no injection surface.
Level of scrutiny
The fix is minimal and mechanical — one property name changed from options.env to options[kBunEnv] to match what the async code path already does. The Windows PATH canonicalization is equally narrow: it only renames a key whose uppercased form equals "PATH" to the canonical string "PATH"; all other keys are unaffected. Both changes follow established patterns already present in the file.
Other factors
Three rounds of review feedback (dynamic require, combined assertion, stderr discarded) were all addressed in subsequent commits, the last of which (5d1f4f4) is the current HEAD. The pre-existing parseEnvPairs split-limit bug I flagged is unrelated to this PR's scope and was noted as such.
There was a problem hiding this comment.
LGTM — all prior feedback addressed, one-line fix is correct and well-tested.
Extended reasoning...
Overview
Two files changed: a one-line fix in spawnSync switching from options.env to options[kBunEnv] (the pre-normalized env snapshot), and a Windows PATH canonicalization addition in normalizeSpawnArguments, plus a new regression test.
Security risks
None. The change routes the already-captured, already-validated env snapshot to the sync spawn path — consistent with what the async path has always done. No new surface area.
Level of scrutiny
Low. The async path (ChildProcess.spawn) already used options[kBunEnv]; this simply applies the same pattern to the sync path. The Windows PATH fix is a narrow case-insensitive rename with no security implications.
Other factors
All prior review comments have been resolved: require() calls moved to top-level imports, stderr: "pipe" restored for CI diagnostics, flaky not.toContain("error:") assertion removed. No bugs were flagged by the automated bug hunter. CI failures cited in the PR are infrastructure flakes unrelated to this change.
|
CI note: the only remaining failure on build #45459 is |
1 participant
Fixes #29237
Problem
execFileSync,spawnSync, andexecSyncwere resolving bare command names against a stale snapshot of the environment when the caller omitsoptions.env— so runtime mutations toprocess.env.PATHwere ignored. Node honors them; Bun didn't. Passingenv: process.envexplicitly worked around it.Repro
Cause
normalizeSpawnArguments(insrc/js/node/child_process.ts) readsoptions.env || process.envlive, copies it into a freshbunEnvobject, and stashes that on the returned options as a symbol-keyed property[kBunEnv]. The async path (ChildProcess.spawn, line 1308) correctly reads:The sync path (
spawnSync, line 537) was reading the wrong property:When the user omits
env,options.envisundefinedandBun.spawnSyncfalls back to the process-start environment snapshot for PATH resolution. The freshly-capturedbunEnvsits unused.Fix
Route
spawnSyncthrough the samekBunEnvsymbol the async path already uses:One-line change — this also fixes
execFileSyncandexecSync, which route throughspawnSync.Verification
New regression test:
test/regression/issue/29237.test.ts— coversexecFileSync,spawnSync,execSync(no explicit env) plus an explicit-env sanity check.FAKE_CALLEDSkipped on Windows (the PATH mutation test needs POSIX-style shebang binaries).
CI note
Build #45425 shows only infra-flake failures on
linux-aarch64-musl-build-cpp(exit -1, agent preemption) and twodarwin-*-aarch64-test-bunjobs stuck inExpiredstate before running. All 52 other jobs pass, including every Windows test variant (verifying the Windows PATH casing fix), ASAN on Linux, and all non-aarch64 macOS/Linux/Windows configurations. No test-level failures.