cli: sync PWD env var with --cwd chdir#30458
Conversation
|
Updated 5:30 AM PT - May 17th, 2026
❌ @robobun, your commit 43bada2 has some failures in 🧪 To try this PR locally: bunx bun-pr 30458That installs a local version of the PR into your bun-30458 --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:
WalkthroughThis PR updates the ChangesPWD Environment Synchronization for --cwd
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/cli/Arguments.zig`:
- Around line 1743-1764: The code updates std.os.environ before checking the
OS-level environment API results; call and check the return value of setenv()
(POSIX) or SetEnvironmentVariableW() (Windows) and only modify std.os.environ
(replace or grow and assign entry_ptr) if the OS call succeeded, or
alternatively propagate the failure by changing the surrounding function's
return type from OOM!void to an error-returning type and return the error on
failure; ensure you reference and handle errors from
bun.c.SetEnvironmentVariableW and setenv("PWD", cwd.ptr, 1) and keep
entry_ptr/std.os.environ unchanged on failure.
In `@test/cli/install/bun-run.test.ts`:
- Around line 333-421: The tests only exercise the direct entrypoint form (cmd:
[bunExe(), "--cwd=subdir", "test.js"]) but the regression is for the `bun
--cwd=... run ...` form; update the three it() cases ("--cwd updates
process.env.PWD...", "--cwd PWD is inherited...", "--cwd adds PWD when parent
had none") to run in both modes by reusing the existing withRun test matrix (use
withRun or the project’s matrix helper) so each test spawns both the
bare-entrypoint command and the run-subcommand variant (e.g., build the cmd
array for the run case like [bunExe(), "--cwd=subdir", "run", "test.js"] or
similar per your helper), altering the Bun.spawn calls in those tests to iterate
the matrix and assert the same expectations for stdout and exitCode for each
variant.
- Around line 368-375: The fixture script string for "subdir/test.js" currently
uses require("child_process") to pull in spawnSync; change it to use an ES
module import at module scope (import { spawnSync } from "child_process") and
remove the inline require call so the rest of the script that calls
spawnSync(process.execPath, [...]) and writes process.stdout remains unchanged;
update the fixture text to reflect the import-based version so it aligns with
ES6 module semantics.
🪄 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: 4b95492e-4b08-466a-837e-82af91cfdbcc
📒 Files selected for processing (2)
src/cli/Arguments.zigtest/cli/install/bun-run.test.ts
There was a problem hiding this comment.
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/cli/install/bun-run.test.ts`:
- Around line 345-347: Replace the manual loop over withRun with a Jest
parameterized describe block: change the for (const withRun of [false, true]) {
... } pattern to describe.each([false, true])("withRun=%s", (withRun) => { ...
}) and move the existing body (including label and buildCmd definitions and the
three test cases) inside that describe callback so tests run with both parameter
values; keep the variables label and buildCmd as-is inside the new describe to
preserve scope and behavior.
🪄 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: cdaa2fd6-4414-4162-a9e6-8a9d32d41e95
📒 Files selected for processing (1)
test/cli/install/bun-run.test.ts
| for (const withRun of [false, true]) { | ||
| const label = withRun ? "bun --cwd run" : "bun --cwd"; | ||
| const buildCmd = (...extra: string[]) => [bunExe(), "--cwd=subdir", ...(withRun ? ["run"] : []), ...extra]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'for \(const withRun of \[false, true\]\)' test/cli/install/bun-run.test.ts
rg -n 'describe\.each\(' test/cli/install/bun-run.test.tsRepository: oven-sh/bun
Length of output: 178
🏁 Script executed:
sed -n '340,370p' test/cli/install/bun-run.test.tsRepository: oven-sh/bun
Length of output: 1504
🏁 Script executed:
sed -n '340,437p' test/cli/install/bun-run.test.tsRepository: oven-sh/bun
Length of output: 3794
🏁 Script executed:
sed -n '140,160p' test/cli/install/bun-run.test.tsRepository: oven-sh/bun
Length of output: 923
Use describe.each() for the withRun parameterization instead of a manual loop.
The test block at line 345 uses for (const withRun of [false, true]) to parameterize three test cases. Convert to describe.each([false, true])("withRun=%s", (withRun) => { ... }) to align with test guidelines and the existing pattern in this file (see line 146).
🤖 Prompt for 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.
In `@test/cli/install/bun-run.test.ts` around lines 345 - 347, Replace the manual
loop over withRun with a Jest parameterized describe block: change the for
(const withRun of [false, true]) { ... } pattern to describe.each([false,
true])("withRun=%s", (withRun) => { ... }) and move the existing body (including
label and buildCmd definitions and the three test cases) inside that describe
callback so tests run with both parameter values; keep the variables label and
buildCmd as-is inside the new describe to preserve scope and behavior.
|
CI red lanes on Windows are the known-flaky `test-http-should-emit-close-when-connection-is-aborted.ts` timeouts, not anything touching this diff — the same three Windows test-bun jobs timing out with the same test hit merged PR #30408 (build 52862) and several others before it. The diff-specific additions in this PR are skipped on Windows ( Not pushing a |
--cwd changes process.cwd() but leaves the inherited PWD pointing at the parent directory. Tools that read PWD for module resolution (vue-tsc / TypeScript) then resolved against the wrong root. Match bash's cd: on a successful chdir, setenv PWD to the new directory. The POSIX path is a plain setenv — bun_sys::environ() reads libc's live environ pointer on each call, so DotEnv::load_process sees the update. Windows needs two writes: SetEnvironmentVariableW for the OS env block (child processes), plus patching the WTF-8 snapshot in bun_core::os::environ() that convert_env_to_wtf8 froze at startup, otherwise DotEnv::load_process still iterates the pre-chdir snapshot. Replace the existing PWD= slot in place when present, or grow the slice by one and re-publish via set_environ. Tests skip on Windows — PWD is a POSIX shell convention; the path-shape (.loose / forward-slash vs \backslash) makes strict-equality tests noisy there. Fixes #30456
robobun
force-pushed
the
farm/97a34fd9/cwd-pwd-env
branch
from
350e68b to
4a1948c
Compare
| unsafe { | ||
| let mut_base = env.as_ptr() as *mut *mut core::ffi::c_char; | ||
| *mut_base.add(i) = entry_ptr; | ||
| } |
There was a problem hiding this comment.
🔴 Writing through env.as_ptr() as *mut *mut c_char is UB under Stacked Borrows / Tree Borrows: env is a &'static [*mut c_char] built via slice::from_raw_parts (src/bun_core/lib.rs:245-253), so .as_ptr() carries SharedReadOnly provenance and the cast doesn't change that — Miri will reject the write at line 1634. The bun_core::os module already stores the raw *mut *mut c_char in ENVIRON precisely to avoid this round-trip; either expose an environ_raw()/environ_mut() accessor and write through that, or just rebuild + set_environ() here the same way the no-PWD branch already does.
Extended reasoning...
What the bug is
patch_windows_environ_snapshot obtains the envp slice via bun_core::os::environ(), which returns a shared reference:
// src/bun_core/lib.rs:245-253
pub unsafe fn environ() -> &'static [*mut c_char] {
let (p, n) = core::ptr::read(&raw const ENVIRON);
if p.is_null() { &[] } else { core::slice::from_raw_parts(p, n) }
}The PR then writes through a pointer derived from that shared reference:
// src/runtime/cli/Arguments.rs:1632-1635
let mut_base = env.as_ptr() as *mut *mut core::ffi::c_char;
*mut_base.add(i) = entry_ptr;<&[T]>::as_ptr() yields a *const T whose provenance is inherited from the &[T] borrow. Under Stacked Borrows that tag is SharedReadOnly; under Tree Borrows the shared reborrow creates a Frozen node. An as-cast to *mut does not grant write permission — it preserves the same tag — so the store at *mut_base.add(i) writes through a read-only tag, which is undefined behavior. Miri will flag this as "attempting a write access using … but that tag only grants SharedReadOnly".
Why existing code doesn't prevent it
The SAFETY: comment on the block argues exclusive access ("single-threaded argv parse … no other thread reads environ yet"). That justifies the absence of data races, but provenance is a per-pointer property independent of threading: even with truly exclusive access, a pointer derived from &T (no UnsafeCell) may not be used to write T. The underlying allocation is in fact writable (it came from Box::leak in convert_env_to_wtf8), but that mutable provenance was discarded the moment the access went through slice::from_raw_parts::<*mut c_char> → &[*mut c_char].
The codebase documents exactly this hazard at src/CLAUDE.md:272-276 ("Pointer provenance at FFI boundaries": "a &self-derived raw pointer carries SharedReadOnly provenance"), and bun_core::os was deliberately designed around it — the comment at lib.rs:221-225 explains that ENVIRON is stored as a raw (*mut *mut c_char, usize) pair specifically so writers don't have to alias a live &mut or round-trip through a shared borrow.
Step-by-step proof
- On Windows,
convert_env_to_wtf8builds aBox<[*mut c_char]>, leaks it, and stores(ptr, len)inENVIRONviaset_environ. The leaked allocation has a Unique root tag with full read/write permission. bun --cwd=subdirruns withPWDinherited (the common case).Arguments::parsecallsset_pwd_env→patch_windows_environ_snapshot.- Line 1624:
environ()reads(p, n)fromENVIRONand callscore::slice::from_raw_parts(p, n). This creates a newSharedReadOnlyborrow (Stacked Borrows) /Frozenchild (Tree Borrows) of the allocation, covering elements[0, n), and returnsenv: &'static [*mut c_char]carrying that tag. - The loop finds
env[i]starting withb"PWD=". - Line 1633:
env.as_ptr()returns*const *mut c_charwith the sameSharedReadOnlytag asenv. Casting itas *mut *mut c_charchanges only the static type; the tag is unchanged. - Line 1634:
*mut_base.add(i) = entry_ptrperforms a write at offsetiusing a tag that grants only read permission → UB. Under Miri this aborts; under rustc the optimizer is free to assumeenv[i]is unchanged for the lifetime ofenv(it's still live through thereturn) and could e.g. reorder or eliminate the store.
Impact
This is textbook aliasing-model UB in new unsafe code introduced by this PR, on the path taken by every Windows --cwd invocation where PWD is inherited. Practical miscompilation risk with current rustc/LLVM is low (the &[T]-derived noalias is read-only and there's no competing read after the write here), but the project's own conventions explicitly forbid this pattern, the surrounding module was designed to make the correct approach easy, and the new tests skipIf(isWindows) so this path has zero CI coverage.
Suggested fix
Don't round-trip through the &[..] view for the write. Two easy options:
- Use the existing primitive: the no-
PWDbranch in this same function already does it correctly — build a freshBox::leak'd array and publish it viabun_core::os::set_environ(ptr, len). The in-place branch can do the same (copyenv, replace sloti, push the trailing null,set_environ). This costs one small allocation on a one-time startup path. - Add a raw accessor: expose
pub unsafe fn environ_raw() -> (*mut *mut c_char, usize)inbun_core::os(it's justcore::ptr::read(&raw const ENVIRON)) and write through that pointer, which retains the originalBox::leakmutable provenance:let (base, n) = unsafe { bun_core::os::environ_raw() }; for i in 0..n { let bytes = unsafe { bun_core::ffi::cstr_bytes(*base.add(i) as *const _) }; if bytes.starts_with(b"PWD=") { unsafe { *base.add(i) = entry_ptr; } return; } }
| #[cfg(windows)] | ||
| unsafe extern "C" { | ||
| fn SetEnvironmentVariableW(name: *const u16, value: *const u16) -> i32; | ||
| } |
There was a problem hiding this comment.
🟡 nit: SetEnvironmentVariableW is a kernel32 stdcall API, so this should be unsafe extern "system" rather than extern "C" — matching the documented convention at src/windows_sys/externs.rs:1306 ("kernel32 stdcall — use extern "system"") and the sibling GetEnvironmentVariableW declaration at src/sys/windows/mod.rs:4886. On x86_64/aarch64 Windows the two ABIs are identical so there's no runtime impact on supported targets, but it's a one-word fix for consistency. (The same pre-existing pattern exists at src/runtime/node/node_process.rs:138-143, which is probably worth fixing alongside.)
Extended reasoning...
What
The new declaration at src/runtime/cli/Arguments.rs:1662-1665 is:
#[cfg(windows)]
unsafe extern "C" {
fn SetEnvironmentVariableW(name: *const u16, value: *const u16) -> i32;
}SetEnvironmentVariableW is a kernel32 Win32 API, and Win32 APIs use the stdcall calling convention. In Rust, the correct ABI specifier for this is extern "system", which resolves to stdcall on i686-pc-windows and to the platform C ABI on x86_64/aarch64 (where stdcall and the C ABI coincide). The codebase already documents and follows this convention:
src/windows_sys/externs.rs:1306carries an explicit comment: "kernel32 stdcall — use extern "system" so the callconv is correct on all targets", and every kernel32 declaration in that file usesextern "system".src/sys/windows/mod.rs:4886declares the siblingGetEnvironmentVariableW/GetEnvironmentStringsW/FreeEnvironmentStringsWunderunsafe extern "system".src/jsc/btjs.rs:484declaresGetEnvironmentVariableWunderunsafe extern "system".
Why this is only a nit
On the Windows targets Bun actually ships (x86_64-pc-windows-msvc and aarch64-pc-windows-msvc), extern "C" and extern "system" lower to the same Win64 calling convention — there is exactly one calling convention on those platforms. So this declaration produces correct code today and there is zero runtime impact on any supported target. Additionally, the same crate already has a pre-existing unsafe extern "C" { fn SetEnvironmentVariableW(...) } at src/runtime/node/node_process.rs:138-143, so the PR is following local (if non-ideal) precedent rather than introducing a brand-new inconsistency.
The only target where the distinction matters is 32-bit x86 Windows, where extern "C" = cdecl (caller cleans stack) and extern "system" = stdcall (callee cleans stack). Calling a stdcall function through a cdecl prototype there would mis-balance the stack by 8 bytes (two pointer args). Bun does not target i686-pc-windows, so this is a latent portability hazard rather than a live bug.
Step-by-step
bun --cwd=subdir ...runs on Windows;Arguments::parsechdir's successfully and callsset_pwd_env(line 821).set_pwd_enventers the#[cfg(windows)]arm and callsSetEnvironmentVariableW(PWD_W.as_ptr(), wcwd.as_ptr())(line 1597).- The call site is compiled against the
extern "C"prototype at line 1664. On x86_64/aarch64, rustc emits the standard Win64 callconv for both"C"and"system", so the args land inRCX/RDXand the call works correctly — identical machine code to theextern "system"spelling. - On a hypothetical i686 build, rustc would emit a cdecl call (push args, caller pops 8 bytes after return) while kernel32's
SetEnvironmentVariableW@8is stdcall (callee already popped 8 bytes viaret 8). The double-pop misaligns ESP by 8 bytes and corrupts the caller's stack frame.
Impact
None on shipped binaries. This is purely a documented-convention violation in new code, with a one-word fix.
Suggested fix
#[cfg(windows)]
unsafe extern "system" {
fn SetEnvironmentVariableW(name: *const u16, value: *const u16) -> i32;
}Optionally also fix the pre-existing copy at src/runtime/node/node_process.rs:138-143 while in the area.
|
Re-roll (43bada2) hit another unrelated Debian 13 test-bun lane expiring — not test failures, the job hit the build timeout. Previous run had the x64-asan lane SIGKILL (exit -1) on a different test. Neither touches the |
1 participant
Repro
bun --cwd=<dir>callschdir()soprocess.cwd()reports the newdirectory, but
process.env.PWDstill points at the parent. Any tool(or child process) that reads
PWDsees the wrong directory.Cause
arguments::parse(src/runtime/cli/Arguments.rs) only calledbun_sys::chdir— it never updated thePWDenv var.PWDisinherited from the parent shell and left unchanged.
For the reported scenario (
bun --cwd=frontend run lint→vue-tsc),TypeScript's
ts.sysreadsprocess.env.PWDas the resolution rootfor relative module specifiers, so
./App.vueresolves against theproject root instead of
frontend/, and module resolution fails.Bun's shell interpreter already updates
PWDwhen it runs its ownbuiltin
cd, so scripts that go throughbun run→ shell → childrenusually recover. But the Bun process started by
--cwditself — andany
bun --cwd=X -e ...orbun --cwd=X file.js— sees the stalePWD.Fix
After a successful
--cwdchdir, writePWDto the new directory:setenv("PWD", cwd, 1).bun_sys::environ()reads libc'slive
environpointer on every call, soDotEnv::load_processpicks up the change — no further work needed. Matches
bash'sbuiltin
cdbehaviour (only overwrite on success).SetEnvironmentVariableWplus a patch intobun_core::os::environ()— the WTF-8 snapshotconvert_env_to_wtf8freezes at startup(
src/sys/windows/env.rs).SetEnvironmentVariableWonly mutatesthe Win32 env block, so without patching the snapshot
DotEnv::load_processstill sees the pre-chdir value. Replacethe existing
PWD=slot in place when present, or grow theenvp slice by one and re-publish via
bun_core::os::set_environ.Only touched when
--cwdis actually provided.Rebase note
When this PR was first opened the CLI argument parser lived in Zig
(
src/cli/Arguments.zig); the port in #30412 moved the hot path toRust (
src/runtime/cli/Arguments.rs) while leaving the Zig file asdead code. After the rebase the fix was ported to Rust — this is why
the diff is now against
.rsinstead of.zig.Verification
test/cli/install/bun-run.test.ts— three cases ×withRun={false,true}:--cwdupdatesprocess.env.PWDto matchprocess.cwd()PWDis inherited by spawned child processes--cwdpublishesPWDeven when the parent env doesn't have one(
env -u PWD/ cron / systemd / minimal Docker)Skipped on Windows (
PWDis a POSIX shell convention; the path-shape.loose-normalised forward-slash vsprocess.cwd()'s backslashesmakes strict-equality tests noisy there) — the fix still runs, the
test just doesn't assert the cross-platform-dicey shape.
Fixes #30456