Honor live $HOME mutations in os.homedir() - #29248
Conversation
|
Updated 9:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 578bd78 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29248That installs a local version of the PR into your bun-29248 --bun |
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughPOSIX os.homedir() now reflects live changes to process.env.HOME via a JS wrapper; the Zig binding no longer uses a Posix HOME fast-path and always falls back to passwd lookup. Added hardened memory handling in Zig and subprocess-based regression tests validating runtime, inherited, empty, and userInfo() behaviors. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean fix that mirrors the existing pattern; the two flagged issues are minor test-style nits.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility bug where returned a stale value after was mutated at runtime. Three files are touched: the Zig binding (), the JS wrapper (), and a new regression test file.
Security Risks
None. The change only affects how the HOME environment variable is read — moving from Bun's cached env-var accessor to a live lookup on every call, exactly matching what already does. There is no user-controlled input, no new attack surface, and no privilege boundary crossing.
Level of Scrutiny
Low. This is a small, targeted bug fix with a clear root cause (cached env-var snapshot vs. live read) and a well-understood fix pattern already used by . The code path is deliberately left unchanged, preserving Node's behavior of ignoring $HOME there. The regression tests cover the exact failure modes described in the issue.
Other Factors
The two bug reports are both nits about test conventions (missing and assertion ordering), not correctness issues. The implementation is correct and the tests do verify the fix. No CODEOWNERS appear to own these files based on my review.
![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/29244.test.ts`:
- Around line 52-53: Remove brittle empty-stderr assertions and rely on the
subprocess exit code: delete the expect(stderr).toBe("") checks in this test
(and the other occurrences noted) and keep/assert only expect(exitCode).toBe(0);
if stderr content is the actual behavior under test then replace the
empty-string assertion with a targeted assertion for that expected output,
otherwise do not assert on the stderr variable. Use the stderr and exitCode
variable names from the test to locate and update the assertions.
🪄 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: b998db56-47a1-411d-8221-0fefaded7bad
📒 Files selected for processing (1)
test/regression/issue/29244.test.ts
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
LGTM — all substantive bugs addressed, the nit is cosmetically redundant but behaviorally harmless.
Extended reasoning...
Overview
The PR moves the POSIX $HOME env check from Bun's cached Zig accessor into the JS layer (src/js/node/os.ts), mirroring the existing tmpdir pattern. The Zig binding becomes the passwd-fallback path only, and userInfo().homedir continues calling the binding directly to match Node's behavior.
Changes reviewed
Three files changed: node_os.zig (remove cached HOME fast-path, fix ERANGE stack-free UB), os.ts (add homedirFactory), and a new regression test file.
Security risks
None. The change reads an environment variable that is entirely user-controlled; no privilege escalation, injection, or data exposure vectors exist.
Prior feedback resolution
All four prior issues I raised were resolved or declined with good justification:
test.concurrentadded (d7564f8)- stdout-before-exitCode assertion order fixed (d7564f8)
- ERANGE guard against freeing stack buffer added (7158e41)
pw.pw_dirnull behavior: author verified libuv unconditionallystrlenspw_dirwith no null check, so Bun is actually more defensive — decline accepted.- Empty
stderrassertions dropped (55c115a)
Remaining nit
The new finding (home.length > 0 dead code on line 44 of os.ts) is a pure cosmetic redundancy with zero behavioral impact; it does not warrant blocking the PR.
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the fix correctly mirrors the tmpdir pattern and the Zig ERANGE guard is properly hardened.
Extended reasoning...
Overview
This PR fixes a bug where returned a stale value after was mutated, by moving the HOME env check from Zig's cached env-var accessor to a live Bun.env["HOME"] read in the JS layer (). The Zig binding becomes the pure passwd-fallback path. Three files are touched: , , and a new regression test.
Security risks
None. There is no auth, crypto, or permissions code involved. The change reads an environment variable that the process already owns.
Level of scrutiny
Moderate. The fix is a well-scoped behavioral correctness fix matching Node's libuv behavior. The ERANGE double-free pre-existing bug was found and fixed in this PR (7158e41), the redundant length check was simplified (35e252a), test ordering and concurrency were corrected (d7564f8, 55c115a). The remaining pre-existing issue (geteuid vs getuid in userInfo for setuid processes) is out of scope for this PR and does not block it.
Other factors
All inline comments from prior review runs have been addressed by the author. The regression tests are well-isolated (subprocess-per-test, concurrent, no shared state). The Windows path correctly delegates to libuv which already reads USERPROFILE live.
|
Note on CI status: the failing lane is |
35e252a to
5fc4531
Compare
There was a problem hiding this comment.
All prior feedback has been addressed and I didn't find anything new, but this changes runtime semantics for os.homedir() and os.userInfo().homedir (now always passwd-backed) plus the geteuid→getuid swap and ERANGE allocator path in Zig — worth a human sanity check before merge.
Extended reasoning...
Overview
This PR makes os.homedir() honor live mutations of process.env.HOME by moving the env check from the Zig binding (which used Bun's snapshot-on-first-read bun.env_var.HOME accessor) into a JS wrapper in src/js/node/os.ts that reads Bun.env["HOME"] on every call. Three files change: src/js/node/os.ts (new homedirFactory wrapper), src/runtime/node/node_os.zig (removed HOME fast-path, fixed an ERANGE free-of-stack-buffer bug, switched geteuid()→getuid() for the passwd lookup), and a new 6-test regression file.
Security risks
None identified. Reading $HOME from the live process environment is exactly what Node/libuv does and introduces no new attack surface. The geteuid()→getuid() change aligns with libuv and is, if anything, the safer choice in setuid contexts (homedir now matches the real user, consistent with the uid field userInfo() already reports).
Level of scrutiny
Moderate-to-high. While each individual change is small and well-reasoned, the combined effect touches a hot, widely-depended-on Node-compat API and bundles several distinct behavior changes into one PR:
os.homedir()now reflects runtime$HOMEmutations (the headline fix).os.homedir()withHOME=""now returns""instead of the passwd entry (new Node-parity, but a change from prior Bun behavior).os.userInfo().homedirnow always returns the passwd entry — previously the Zig binding short-circuited on cached$HOME, so this is a real semantics shift even though it's the Node-correct one.- The
getpwuid_rretry loop's allocator handling was rewritten (guarded free + reset-to-stack between free and alloc). geteuid()→getuid()in the passwd lookup.
Other factors
The PR has been through ~8 rounds of bot review; every raised point was either fixed (test concurrency, assertion ordering, stderr brittleness, redundant length check, ERANGE stack-free UB, geteuid/getuid mismatch, stale doc comments, non-load-bearing delete-HOME test, HOME="" libuv parity) or reasonably declined/deferred (the pre-existing userInfo() username/shell env-sourcing divergence, punted to a follow-up). Test coverage is solid and the Symbol.toPrimitive wiring is preserved via symbolToStringify(out, "homedir"). CI failures noted in the thread (musl build, Windows http timeout) appear unrelated/flaky per the author's log audit. I'm deferring rather than approving because the cumulative behavior surface — especially the userInfo().homedir change and the Zig allocator path — is the kind of thing a maintainer should glance at, not because anything looks wrong.
|
CI build #51218 failures are pre-existing Windows flakes, not caused by this PR:
My PR touches only |
There was a problem hiding this comment.
All prior feedback has been addressed and I didn't find any remaining issues, but this changes os.userInfo().homedir semantics (cached $HOME → always passwd), swaps geteuid→getuid in the passwd lookup, and reworks the Zig ERANGE allocator handling — worth a human sanity-check before merge.
Extended reasoning...
Overview
This PR fixes #29244 by making os.homedir() honor live process.env.HOME mutations, matching Node. It touches three files:
src/js/node/os.ts: newhomedirFactorywrapper that readsBun.env["HOME"]live on every POSIX call (!== undefinedcheck, soHOME=""→""), falling through to the Zig binding only when HOME is absent. Windows delegates straight to the binding.src/runtime/node/node_os.zig: removes the cachedbun.env_var.HOME.get()fast-path so the binding is now pure passwd-fallback on POSIX; changesgeteuid()→getuid()ingetpwuid_r; hardens the ERANGE retry loop (guards the free against the stack buffer, resets to stack buffer between free and alloc so the defer can't double-free on alloc failure); adds extensive doc comments including a flagged WindowsuserInfo()follow-up.test/js/node/os/os.test.js: 6 new concurrent subprocess tests covering mutation before/after require, inherited HOME,HOME="", deleted HOME → passwd, anduserInfo().homedirignoring HOME.
Security risks
None introduced. Reading $HOME from live process.env is standard Node behavior and the same pattern tmpdir() already uses. The geteuid → getuid change is arguably a hardening: in a setuid process, userInfo() now reports a self-consistent {uid, homedir} pair sourced from the real UID, matching libuv.
Level of scrutiny
Medium-high. While the JS wrapper is straightforward and mirrors the existing tmpdir pattern, the PR also makes three non-trivial runtime changes that go beyond the headline fix:
os.userInfo().homedirbehavior change — previously short-circuited via cached$HOMEwhen set; now always reads the passwd entry. Correct (matches Node'suv_os_get_passwd), but user-observable.geteuid→getuid— correct per libuv, but changes which user's passwd entry is consulted in setuid contexts.- Zig allocator rework in the ERANGE loop — fixes a real pre-existing free-of-stack-memory bug, but is memory-safety-critical code.
These are all improvements and look correct to me, but collectively they exceed the "simple/mechanical" bar for bot-only approval.
Other factors
This PR has been through ~8 rounds of bot review; every inline comment is resolved with a corresponding fixup commit, and the author's responses (including the declined libuv-ENOENT claim and the deferred userInfo username/shell follow-up) were well-reasoned. Test coverage is solid: 6 subprocess tests with fail-before/pass-after verification, load-bearing sentinel checks, and the existing Symbol.toPrimitive coverage for homedir still applies via symbolToStringify. The bug-hunting system found nothing in the current revision. CI failures noted in the thread were diagnosed as unrelated Windows/ASAN flakes.
There was a problem hiding this comment.
No issues found, but this touches native Zig allocator handling (the ERANGE retry-loop hardening) plus a few subtle behavioral shifts — userInfo().homedir now always hits getpwuid_r on POSIX, geteuid→getuid, and HOME="" semantics — so it's worth a human pass before merge.
Extended reasoning...
Overview
Three files: src/js/node/os.ts adds a homedirFactory wrapper that reads Bun.env["HOME"] live on every call (mirroring the existing tmpdir pattern) and falls back to the binding; src/runtime/node/node_os.zig removes the cached bun.env_var.HOME.get() fast-path, hardens the getpwuid_r ERANGE retry loop (guards the free against the on-stack buffer and resets to the stack buffer between free and alloc so a failed alloc can't leave a dangling pointer for the defer), and switches the passwd lookup from geteuid() to getuid(); test/js/node/os/os.test.js gains 6 subprocess-isolated concurrent tests covering mutate-after-require, mutate-before-require, inherited HOME, HOME="", deleted HOME → passwd, and userInfo().homedir ignoring HOME.
Security risks
None identified. No injection, auth, or crypto surfaces. The geteuid→getuid change is security-adjacent (setuid processes) but moves Bun toward libuv's behavior and toward internal consistency with the uid field userInfo() already reports, so it's a correctness fix rather than a new exposure.
Level of scrutiny
Medium-high. The JS wrapper is trivial and pattern-matched against tmpdir, but the Zig changes are not mechanical: (1) the ERANGE buffer-free hardening rewrites allocator handling in a retry loop — the logic looks correct and is well-commented, but it's exactly the kind of native memory-management change that benefits from a second pair of eyes; (2) removing the HOME fast-path means userInfo().homedir on POSIX now always issues a getpwuid_r syscall instead of returning the cached env value — more Node-correct, but a real behavioral change; (3) HOME="" now returns "" instead of falling through to passwd, flipping prior Bun behavior to match Node.
Other factors
The PR has been through roughly ten rounds of bot review and every thread is resolved; test coverage for the new behavior is thorough and subprocess-isolated; CI failures noted in the thread are documented Windows flakes unrelated to these files; no CODEOWNERS apply. The remaining userInfo() divergences (POSIX username/shell from env, Windows homedir via uv_os_homedir) are explicitly deferred to a follow-up and called out in the doc comments. Nothing blocks merge from my side — deferring purely because native allocator changes plus three behavioral shifts in one PR exceed the "simple/mechanical" bar for auto-approval.
|
CI update: the Windows x64-baseline test-bun shard pg=7/8 has reliably timed out on |
210f2f0 to
00c44b9
Compare
|
Rebased onto main to resolve the merge conflict. The conflict was non-trivial: main's Zig→Rust migration deleted
Gate re-verified on the rebased tree: 4/6 tests fail without the fix, all 6 pass with it. History collapsed to a single commit since the original Zig-targeting commits no longer apply to the ported file. |
|
Post-rebase CI (build #64856): the diff is green. 45 lanes passed; the only failure is That's the test-bun step unable to download the compiled binary artifact from the build-bun step (S3/agent hiccup), unrelated to this PR's 3-file change. The homedir tests themselves pass locally (gate: 4/6 fail without the fix, 6/6 pass with it). A maintainer can retry just that one job rather than re-running all lanes. Not pushing an empty retrigger commit since it would reset the 45 lanes that already passed. |
There was a problem hiding this comment.
LGTM — the rebase onto the Rust port carries over the fix cleanly and all prior review feedback is addressed.
Extended reasoning...
Overview
This PR fixes #29244: os.homedir() was returning a stale value after process.env.HOME was mutated at runtime, because the native binding read HOME through Bun's snapshot-on-first-read env-var cache. The fix moves the live HOME check into src/js/node/os.ts (mirroring the existing tmpdir pattern) and reduces the native binding to the passwd-fallback path. Three files touched: src/js/node/os.ts (+35 lines, mostly comments, adding homedirFactory), src/runtime/node/node_os.rs (−8/+18: remove the cached env_var::HOME fast-path, switch geteuid()→getuid() to match libuv, add explanatory comments), and test/js/node/os/os.test.js (+119: six subprocess-isolated tests under describe("homedir live \$HOME mutations")).
The PR has been through ~10 rounds of automated review since April. Every finding I raised — test concurrency, assertion ordering, ERANGE free-of-stack-pointer, HOME="" vs unset semantics, stale doc comments, sentinel-based passwd-fallback test, geteuid/getuid consistency, test file location, POSIX-only qualification of the userInfo() parity claim — was addressed and resolved. The two remaining items (POSIX userInfo() username/shell sourcing, Windows userInfo().homedir honoring USERPROFILE) are pre-existing divergences explicitly deferred to a follow-up and now documented inline. The most recent commit is a rebase onto main's Zig→Rust migration of node_os; the author re-applied the (now much smaller) native-side change to the Rust port and re-verified the gate (4/6 tests fail without the fix, 6/6 pass with it). The Rust port's Vec<u8> heap buffer makes the earlier Zig ERANGE allocator fix moot.
Security risks
None identified. The change reads an environment variable the process already controls and passes it back verbatim — no shell, no filesystem, no privilege boundary crossed. The geteuid()→getuid() switch aligns with libuv and, if anything, makes setuid behavior more predictable (homedir and uid now describe the same user).
Level of scrutiny
Moderate. os.homedir() is widely used, but the JS-side change is a thin wrapper that strictly adds a live env read before the existing fallback — behavior only changes for callers who mutate HOME at runtime (the bug being fixed) or set HOME="" (now returns "" like Node instead of falling through to passwd). The native diff is tiny: an 8-line deletion, a one-identifier change, and comments. The new tests are subprocess-isolated, concurrent, Windows-skipped, and load-bearing for the regression class.
Other factors
No CODEOWNERS cover these paths. No outstanding unresolved review comments. The bug-hunting system found nothing on the post-rebase revision. Existing os.test.js coverage (including the Symbol.toPrimitive coercion suite that exercises homedir) continues to pass per the PR description. CI failures noted in the thread were unrelated Windows HTTP-test flakes confirmed on other PRs. Given the small, well-understood diff, exhaustive prior review, and verified test gate, this is safe to approve.
|
Thanks for the review. Confirming the current state for a maintainer: the fix is ported to the Rust |
|
CI update (build #64856): all 72 individual lanes now pass, including So the diff is fully green on every real lane. The stale aggregate just needs a maintainer to re-run the build or merge past it; I'm not pushing an empty retrigger since that would reset all 72 passing lanes. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased on main (670d8ce) and trimmed comment verbosity; all homedir tests pass locally. Note: #35270 (the full |
On win32 the factory returned a function that only forwarded to the binding; return the binding itself instead. Compress the remaining comments to one sentence each.
|
Addressed the comment-cop findings in 5a280e5:
All tests pass, including the Symbol.toPrimitive coercion suite. |
|
Note: the walkthrough above is out of date — it describes the pre-rebase diff (
Head is 5a280e5, all review threads resolved. |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/node/node_os.rs:751-756— The new comment claims "libuv's uv__getpwuid_r uses the real uid" to justify switchinggeteuid()→getuid(), but that's factually wrong — libuv'suv_os_get_passwd()(which backs both Node'sos.homedir()passwd fallback andos.userInfo()) callsuv__getpwuid_r(pwd, geteuid()), and the libuv docs say "the current effective uid (not the real uid)". Before this PR the code usedgeteuid()and matched Node/libuv; this change (from earlier review comment #6, which was itself unverified) now diverges from Node in setuid processes while claiming to match it. Revert tolibc::geteuid()and fix the comment; if consistency withuserInfo().uidis wanted, that field should also move togeteuid()in the deferred follow-up.Extended reasoning...
What the bug is
Commit 627e2c3 (carried forward into the Rust port) changed the passwd lookup in
homedir()fromlibc::geteuid()tolibc::getuid(), and added a comment atsrc/runtime/node/node_os.rs:751-752:// libuv's uv__getpwuid_r uses the real uid; userInfo() below reports
// uid = getuid(), so geteuid here would desync them under setuid.The first clause is factually false. libuv's
uv_os_get_passwd()— which is what Node'sos.userInfo()wraps, and whatuv_os_homedir()falls back to whenHOMEis unset — is defined in libuvsrc/unix/core.cas:int uv_os_get_passwd(uv_passwd_t* pwd) { return uv__getpwuid_r(pwd, geteuid()); }
and the libuv API docs for
uv_os_get_passwdstate: "Gets a subset of the password file entry for the current effective uid (not the real uid)." Anduv_os_homedir()'s fallback path callsuv_os_get_passwd(), so it too keys ongeteuid().The specific code path
- Before this PR:
node_os.rscalledlibc::getpwuid_r(libc::geteuid(), …)— matching libuv/Node exactly. - Earlier review comment (previous_comments #6, resolved in 627e2c3) asserted "to match libuv" and requested
geteuid()→getuid(). That assertion was never verified against libuv source and is wrong. - This PR applied the change and added a comment enshrining the incorrect claim.
- Now, in a setuid process where
getuid() ≠ geteuid(), Bun'sos.homedir()(withHOMEunset) andos.userInfo().homedirreturn the passwd entry for the real user, whereas Node returns the passwd entry for the effective user.
Why nothing catches it
Under normal execution
getuid() == geteuid(), so all tests pass identically. No test exercises a setuid binary. The comment's factual claim is not checked by anything. REVIEW.md is explicit on this class: "Verify semantics empirically, never from names or intuition… For ported code, the reference implementation (esbuild, Node) is the spec — diff control flow against it before 'fixing' apparent bugs." This is exactly a case where an unverified claim about the reference led to changing correct code to incorrect code.Step-by-step proof
- Setuid binary: real uid = 1000 (
alice,pw_dir=/home/alice), effective uid = 0 (root,pw_dir=/root).HOMEis unset. - Node:
os.homedir()→uv_os_homedir→getenv("HOME")is NULL →uv_os_get_passwd→uv__getpwuid_r(pwd, geteuid()=0)→ returns/root.os.userInfo().homedir→ same path →/root, andos.userInfo().uid→pwd.uid= 0. - Bun before this PR:
getpwuid_r(geteuid()=0, …)→/root. ✅ Matches Node. - Bun after this PR:
getpwuid_r(getuid()=1000, …)→/home/alice. ❌ Diverges from Node. - The comment says this change was made "to match libuv" — it does the opposite.
The "internal consistency" half of the rationale ("userInfo() below reports uid = getuid()") points at a pre-existing Bun divergence: Node's
userInfo().uidispwd.uidfrom thegeteuid()-keyed lookup (i.e. the effective uid), notgetuid(). So the change makeshomedirconsistent with Bun's wronguidrather than fixing both toward Node. That pre-existinguid/username/shelldivergence is already flagged for the deferred follow-up PR.Impact
- Comment: enshrines misinformation about the reference implementation right next to the call it justifies; future maintainers will read "libuv uses the real uid" as authoritative.
- Behavior: newly-introduced Node divergence in setuid processes for both
os.homedir()(HOME unset) andos.userInfo().homedir. Narrow surface, but a regression in previously-correct code.
How to fix
Revert to
libc::geteuid()and correct the comment (or delete it — the surrounding docblock already says this path "match[es] Node's uv_os_get_passwd", which is sufficient once the call actually does):// libuv's uv_os_get_passwd keys on the *effective* uid (geteuid()); Node's // os.userInfo() and os.homedir() passwd fallback both go through it. let ret = unsafe { libc::getpwuid_r( libc::geteuid(), …
If internal consistency with
userInfo().uidis desired, that field should also move to the effective uid — but that belongs in the already-deferredusername/shell/Windows-userInfofollow-up, not here. - Before this PR:
Verified against libuv v1.x src/unix/core.c: uv_os_get_passwd calls uv__getpwuid_r(pwd, geteuid()), and uv_os_homedir's fallback goes through it. The earlier switch to getuid() was based on an incorrect claim about libuv and diverged from Node in setuid processes. Also trim the test header to the issue URL plus the invariants and drop the stale Zig reference.
|
Both findings addressed in 0cd7f84:
All 9 homedir tests pass (including the Symbol.toPrimitive suite). |
|
CI note on build #82132: the |
There was a problem hiding this comment.
LGTM — the 0cd7f84 pass addressed the prior feedback (header trimmed, geteuid() restored per libuv). One leftover comment-only nit inline; non-blocking.
Extended reasoning...
Overview
Three-file Node-compat fix: src/js/node/os.ts gains homedirFactory reading Bun.env["HOME"] live on every POSIX call (mirroring the existing tmpdir pattern); src/runtime/node/node_os.rs drops the cached env_var::HOME.get() fast-path so the binding is the pure passwd fallback (which userInfo() already calls directly); six subprocess tests added to test/js/node/os/os.test.js.
Delta since last review
0cd7f84 applied both prior findings: the describe-block header was trimmed to issue URL + invariants (stale Zig reference gone), and the passwd lookup was reverted to geteuid() after verifying against libuv's uv_os_get_passwd source. The one remaining nit this run — a single "Previously Bun treated…" sentence in the empty-string test's per-test comment — is the same bug-history class that slipped past the header trim. Comment-only, non-blocking.
Security risks
None. Reads an env var and calls getpwuid_r; no new external input surface, no privilege boundaries crossed.
Level of scrutiny
Low-to-moderate. Small, focused behavior change following an established in-file pattern (tmpdir). Windows path is unchanged (uv_os_homedir already reads USERPROFILE live). Symbol.toPrimitive coercion still works — symbolToStringify(out, "homedir") attaches to the returned closure/binding, and the existing #10259 suite covers it. The env_var import in node_os.rs remains live via USER/SHELL in user_info().
Other factors
All prior review threads resolved; the deferred pre-existing divergences (POSIX username/shell from env, Windows userInfo().homedir via uv_os_homedir) were correctly scoped out to a follow-up. Tests are subprocess-isolated, sentinel-based, and gate-verified (4/6 fail without the fix).
|
Addressed the last nit in c06bfae: dropped the bug-history sentence from the empty-HOME test comment, keeping the libuv contract lines. All homedir tests pass; every review thread is resolved. |
There was a problem hiding this comment.
LGTM — small, focused Node-compat fix that mirrors the existing tmpdir pattern.
What was reviewed:
homedirFactoryin os.ts:Bun.env["HOME"]live read on POSIX,home !== undefinedsoHOME=""→""(matches libuv); Windows returns the binding unchanged, sosymbolToStringifybehavior is identical to before.node_os.rs: removed env fast-path leavesgetpwuid_r(geteuid(), …)as the passwd fallback;env_varimport still live viaUSER/SHELLinuser_info();userInfo().homedircorrectly still skips$HOME.- Tests: 6 subprocess cases with
it.concurrent, stdout drained beforeexited,stderr: "inherit"so no pipe-fill risk; all prior comment-cop / bug-history / file-placement nits addressed through c06bfae.
Extended reasoning...
Overview
Three-file change fixing #29244: os.homedir() returned a stale value after process.env.HOME was mutated because the native binding read HOME through bun_core::env_var::HOME.get(), a snapshot-on-first-read cache. The fix moves the live $HOME check into src/js/node/os.ts (reading Bun.env["HOME"] on every call, exactly like tmpdir does immediately above it) and reduces the native homedir() in src/runtime/node/node_os.rs to the pure passwd/getpwuid_r fallback. Six subprocess tests are added to test/js/node/os/os.test.js covering mutate-after-require, mutate-before-require, inherited HOME, HOME="", deleted HOME → passwd, and userInfo().homedir still ignoring $HOME.
Security risks
None. No user input reaches a syscall or path operation any differently than before; the only behavioral change is which store $HOME is read from (JS-side Bun.env vs a cached native accessor). The passwd lookup path (getpwuid_r) is unchanged apart from the removed env short-circuit. No auth, crypto, permissions, or parsing surface is touched.
Level of scrutiny
Low-to-moderate. This is a Node.js compatibility fix in a well-understood API (os.homedir()), the JS-side change is a direct copy of the tmpdir pattern in the same file, and the Rust change is a net deletion (removing a fast-path) plus a one-line comment. On Windows the exported homedir is byte-identical to before (homedirFactory returns binding.homedir directly, and symbolToStringify already handles native bindings for freemem/hostname/etc.). The Symbol.toPrimitive coercion suite in the same test file continues to cover the wrapped function. userInfo() still calls the native binding directly, which now correctly skips $HOME on POSIX — an improvement over the pre-PR behavior.
Other factors
This PR has been through extensive iteration: file placement (moved from test/regression/ into os.test.js), HOME="" semantics verified against Node 25, getuid vs geteuid verified against libuv source (reverted to geteuid to match uv_os_get_passwd), comment verbosity trimmed per comment-cop, and all bug-history prose removed. Every one of my prior inline nits and the comment-cop findings are resolved as of c06bfae. The env_var import remains live (used by user_info for USER/SHELL). Tests follow harness conventions: bunEnv spread, it.concurrent for independent subprocesses, await using for cleanup, stdout asserted before exit code, stderr: "inherit" avoids the pipe-drain requirement. The known Windows userInfo().homedir divergence (honors USERPROFILE via uv_os_homedir where Node uses uv_os_get_passwd) is pre-existing, documented in the review thread, and correctly deferred to a follow-up alongside the POSIX username/shell divergence — it is not introduced or worsened by this change.
|
Two notes from re-checking this against main (9a543cc), where it still merges cleanly and the bug still reproduces:
Related: #34629 changes the same |
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
Use node:test and make sure the test behaves the same in Node.js that way
Move the six homedir//root tests into os-homedir-env.test.mjs using node:test + node:assert and process.execPath, so the identical file runs under Node.js: node test/js/node/os/os-homedir-env.test.mjs # 6 pass on v26.3.0 Verified fail-before/pass-after under bun bd (4/6 fail without the fix).
|
Done in 58e7023. The six homedir tests now live in Verified under Node v26.3.0 directly: Same 6 pass under |
Plain .test.js runs under bun's test runner and directly under node (test/ is type: commonjs). 6/6 pass on Node v26.3.0 and bun bd.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/node/os/os-homedir-env.test.mjs:4-9— Commit 58e7023 moves the 6 homedir tests back out ofos.test.jsinto a newos-homedir-env.test.mjs, reversing the c05b47a fix this PR already accepted for the earlier "tests belong in the module's existing file" review, and in doing so drops two other harness conventions that were also settled:{...process.env}instead ofbunEnv(REVIEW.md "SpreadbunEnvwhen modifying it") and 6 serialspawnSynccalls (REVIEW.md "async spawns overspawnSync" / test/CLAUDE.md "Prefer concurrent tests"). The Node-parity rationale is reasonable and there is precedent fornode:testundertest/js/node/, but it doesn't require undoing three already-resolved review threads — nit only, no functional impact.Extended reasoning...
What the issue is
Commit 58e7023 ("port homedir env tests to node:test for direct Node parity") moved the 6
os.homedir()env tests out oftest/js/node/os/os.test.jsand into a new standalonetest/js/node/os/os-homedir-env.test.mjsthat usesnode:test+spawnSync(process.execPath, …, { env: { ...process.env } }). This reverses c05b47a — the fix this same PR applied in response to the 2026-05-04 review comment ("these 6 tests belong intest/js/node/os/os.test.js… rather than a new file"), which was accepted and marked resolved with "Moved the 6 tests into test/js/node/os/os.test.js under a describe block".Reverting to a separate file also drops two other harness conventions that earlier review rounds on this PR had settled:
bunEnv: REVIEW.md "Copy harness conventions exactly" says "SpreadbunEnvwhen modifying it". The new file spawns with{ ...process.env }, so children don't getBUN_DEBUG_QUIET_LOGS=1,NO_COLOR=1,JSC_useJIT: undefined, etc. fromharness.ts:64-85.spawnSync/ concurrency: REVIEW.md's same section says "async spawns overspawnSync" and "test.concurrentfor independent subprocess suites". The 6 serialspawnSynccalls can't be made concurrent, regressing the earliertest.concurrentreview fix that was also accepted on this PR.
Why the stated rationale doesn't require this shape
The header comment says "Uses node:test so the same file runs under Node.js to verify parity". That's a legitimate goal, and there is precedent for
node:testundertest/js/node/(e.g.node-tls-connect-hostname-verification.test.ts,node-http2-continuation.test.ts,child-process-rlimit-nofile.test.ts) — so this is not unprecedented. But the repo's dedicated location for run-under-Node parity istest/js/node/test/parallel/, and the other.test.mjsfiles that live alongside module tests (e.g.test/js/node/util/parse_args/*.test.mjs) still import frombun:test+harness. Wanting a Node-runnable copy doesn't require moving the tests out ofos.test.jsand droppingbunEnv/async-spawn — a smalltest/js/node/test/parallel/-style shim could carry the parity check while the primary tests stay where c05b47a put them.Practical impact
Limited. Debug logs go to stderr and the file only
JSON.parses stdout (stderr is only surfaced in the assertion message on non-zero exit), so leakedBUN_DEBUG_*output wouldn't actually break the assertions. The 6 serial spawns add a few hundred ms to the file vs. concurrent async spawns. This is file-organization / harness-convention only — no effect on what the tests verify.Step-by-step
test/CLAUDE.mdsays: "Add tests to the existing test file for the code you're changing — do not create a new file." The existing file istest/js/node/os/os.test.js.- Review on 2026-05-04 flagged the separate-file placement; c05b47a moved the 6 tests into
os.test.jsand the thread was resolved. - 58e7023 creates
os-homedir-env.test.mjsand removes the tests fromos.test.js(verified:grep 29244 os.test.js→ no matches). - The new file uses
spawnSync+{ ...process.env }instead of asyncBun.spawn+bunEnv, contrary to REVIEW.md's "Copy harness conventions exactly" bullets. - ⇒ Three previously-accepted review fixes on this PR (file placement,
bunEnv, concurrent async spawns) are dropped in one commit.
How to fix
Either:
- Keep the tests in
os.test.js(as they were after c05b47a) usingBun.spawn+bunEnv+test.concurrent, and if a Node-runnable parity check is wanted, add a small shim undertest/js/node/test/parallel/; or - Keep the separate
.test.mjsbut note in the PR why re-opening the resolved file-placement thread is preferable, and at minimum spread the harness env ({ ...process.env, BUN_DEBUG_QUIET_LOGS: "1", NO_COLOR: "1" }) so children match whatbunEnvwould set — the same patternchild-process-rlimit-nofile.test.tsuses.
Nit — non-blocking; conventions only.
No harness import is possible (the file runs under plain node), so set BUN_DEBUG_QUIET_LOGS/NO_COLOR explicitly like bunEnv would.
|
On the conventions note: the separate file, plain The one compatible improvement is applied in 578bd78: the spawn env now sets |
There was a problem hiding this comment.
LGTM — small, well-scoped Node-compat fix mirroring the existing tmpdir pattern.
What was reviewed:
homedirFactoryin os.ts: liveBun.env["HOME"]read on POSIX, binding passthrough on Windows;Symbol.toPrimitivewiring unchanged (Windows still attaches to the native fn as before).node_os.rs: removal of the cachedenv_var::HOMEfast-path leaves the binding as pure passwd fallback;env_varimport still live viauser_info();geteuid()matches libuv'suv_os_get_passwd.- Test file: 6 subprocess cases run under both Bun and Node via
node:test+process.execPath; empty-string vs absent HOME distinction, delete-then-fallback, anduserInfo().homedirnon-honoring all covered.
Extended reasoning...
Overview
Three files: src/js/node/os.ts gains a ~15-line homedirFactory that reads Bun.env["HOME"] live on every POSIX os.homedir() call (mirroring the existing tmpdir implementation in the same file) and returns the native binding unchanged on Windows. src/runtime/node/node_os.rs drops the 6-line cached env_var::HOME.get() fast-path so the binding is now the pure getpwuid_r fallback, and adds a one-line comment pinning the geteuid() choice to libuv. test/js/node/os/os-homedir-env.test.js is a new 90-line node:test file with 6 subprocess-isolated cases, verified to pass under both bun bd test and node directly.
Security risks
None identified. The change reads an environment variable and returns it as a string; no path traversal, no filesystem access, no privilege operations added. The geteuid() vs getuid() question was already litigated in prior review rounds and settled by direct reference to libuv source (uv_os_get_passwd uses geteuid()).
Level of scrutiny
Low-to-medium. This is a Node-compat behavioral fix in a non-hot-path getter, following an established in-file pattern (tmpdir). The Rust change is purely subtractive on the POSIX branch. The PR has been through ~10 rounds of automated review over four months with every thread resolved; the remaining diff is minimal and each earlier finding (test placement, comment verbosity, geteuid/getuid, empty-vs-unset HOME semantics, Windows userInfo divergence scoping) has a documented resolution.
Other factors
All prior inline threads are resolved. The author's own 2026-08-12 note about a Worker-env test and a Windows JS-side USERPROFILE check are self-identified follow-ups (scope expansion), not unaddressed review feedback, and don't block this fix. The test file's use of spawnSync/no-harness is intentional so it runs unmodified under Node (per the author's explanation), and it sets BUN_DEBUG_QUIET_LOGS/NO_COLOR inline to compensate for the absent bunEnv. The gate (4/6 fail without the fix, 6/6 pass with it, 6/6 pass under Node) has been re-verified after each substantive change. The overlap with #34629 is a rebase-coordination note only.
There was a problem hiding this comment.
LGTM — small, well-tested Node-compat fix that mirrors the existing tmpdir pattern.
What was reviewed:
homedirFactoryin os.ts:Bun.env["HOME"]live read on POSIX with!== undefinedgate (soHOME=""returns"", matching libuv); Windows returns the binding unchanged, soSymbol.toPrimitivewiring is identical to before.node_os.rs: removing the cachedenv_var::HOMEfast-path leavesuserInfo().homedircorrectly reading the passwd entry viageteuid()(matches libuv'suv_os_get_passwd);env_varimport stays live viaUSER/SHELL.- The 6 node:test subprocess cases cover mutate-after/before-require, inherited env, empty vs. deleted
HOME, and theuserInfo()negative — verified runnable under Node per the parity constraint.
Extended reasoning...
Overview
Three files: src/js/node/os.ts adds a ~12-line homedirFactory that reads Bun.env["HOME"] live on POSIX and delegates to the native binding on Windows; src/runtime/node/node_os.rs removes the ~8-line cached env_var::HOME fast-path so the binding is a pure getpwuid_r(geteuid()) fallback (also used by userInfo()); and a new 90-line test/js/node/os/os-homedir-env.test.js with 6 subprocess tests written against node:test/node:assert so the same file runs under Node.js for parity verification.
Security risks
None. No auth, crypto, permissions, or untrusted-input parsing. Reading process.env.HOME live is exactly what Node/libuv do; the passwd fallback path is unchanged except for dropping the env short-circuit.
Level of scrutiny
Low-to-medium. This is a narrow Node-compat fix following an established in-file pattern (tmpdir already reads Bun.env live in the same way). The Rust change is a pure deletion plus a one-line comment. The PR has been through many review rounds — every prior finding (empty-string semantics, geteuid vs getuid, test placement, comment verbosity, node:test portability) is resolved and reflected in the current diff.
Other factors
- All prior inline threads from claude, comment-cop, and coderabbit are marked resolved; no human reviewer has outstanding comments.
- The bug-hunting system found nothing on this revision.
- The Windows
userInfo().homedir/USERPROFILEdivergence and the Worker-env test suggestion were explicitly deferred to follow-up work earlier in the thread; they are pre-existing behavior this PR does not touch and do not block the #29244 fix. - Confirmed
symbolToStringify(out, "homedir")still works: on Windows it decorates the same native function as before, and on POSIX it decorates the JS wrapper whose()call returns the string. - Overlap with #34629 is a merge-order concern only, not a correctness issue for this diff.
|
Build #93658: 188/196 lanes green. The only failures are darwin x64 build-cpp/build-bun — the agent could not download the prebuilt WebKit tarball ( |
2 participants
Fixes #29244
Repro
Node:
Bun (before this change):
A stronger form — mutate
HOMEbeforerequire('node:os')— also returned the stale value.Cause
src/bun.js/node/node_os.zig'shomedir()readHOMEviabun.env_var.HOME.get(), which is the type-safe cached env-var accessor fromsrc/env_var.zig. OnceHOMEis read, its value is atomically cached and subsequent.get()calls return the cached value — they never re-query the process environment. So runtime mutations ofprocess.env.HOMEwere invisible toos.homedir().os.tmpdir()doesn't have this bug because it readsBun.env["TMPDIR"]live insrc/js/node/os.tson every call.Fix
Mirror the
tmpdirpattern forhomedir. TheHOMEenv check is now done insrc/js/node/os.tsviaBun.env["HOME"], which readsprocess.envlive on every call. The Zig binding is now the passwd-fallback path only (what libuv'suv_os_homedirdoes whenHOMEis unset/empty).os.userInfo().homediris unchanged: it still calls the Zig binding directly, which skips the env check and reads the passwd entry — matching Node's behavior of ignoring$HOMEthere.On Windows, libuv's
uv_os_homediralready readsUSERPROFILElive on every call via syscall (not through Bun's cache), so the binding is called directly on Windows.Verification
test/regression/issue/29244.test.tsruns each case in a subprocess (so mutatingprocess.env.HOMEdoesn't affect the test runner):os.homedir()reflectsHOMEmutated afterrequire— fail-before, pass-afteros.homedir()reflectsHOMEmutated beforerequire— fail-before, pass-afteros.homedir()honorsHOMEinherited from parent envos.homedir()falls back to passwd whenHOMEis emptyos.userInfo().homedirstill ignoresHOMEmutationsExisting
test/js/node/os/os.test.jshomedirtests (includingSymbol.toPrimitive/ template-literal coercion) continue to pass.Related: #29237 / #29239 — similar stale-env-snapshot bug in child_process sync variants.
no test proof · iteration 16 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/os/os-homedir-env.test.js