process.env: Node-semantics exotic object on POSIX; coerce/validate/setenv-sync, first-wins dup load, typed-cache invalidate - #35882
Code review found 2 important issues
Found 5 candidates, confirmed 5. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | test/js/node/process/process-env-exotic.test.ts:137-140 |
getenv FFI probe hardcodes libc.so.6; fails on musl CI |
| 🔴 Important | src/js/builtins/ProcessObjectInternals.ts:561 |
Windows defineProperty trap does not reject Symbol values |
| 🟡 Nit | test/js/node/process/process-env-exotic.test.ts:30-33 |
process.env test cleanup runs after assertions instead of in try/finally |
| 🟡 Nit | test/js/node/process/process-env-exotic.test.ts:216-218 |
execve launcher tests fail instead of skip when no C compiler is present |
| 🟡 Nit | src/runtime/api/BunObject.rs:2254-2256 |
Stale doc comment claims Bun__ProcessEnv__put returns a value |
Annotations
Check failure on line 140 in test/js/node/process/process-env-exotic.test.ts
claude / Claude Code Review
getenv FFI probe hardcodes libc.so.6; fails on musl CI
The `getenvProbe` template hardcodes `"libc.so.6"` for non-Darwin POSIX, which does not exist on musl (Alpine) — Bun's `-musl` CI lanes will fail both getenv-probe tests with a `dlopen` error instead of testing the feature. Import `libcPathForDlopen` from `harness` and interpolate it into the `-e` string via `dlopen(${JSON.stringify(libcPathForDlopen())}, {...})`, exactly as `test/js/bun/net/socket.test.ts:1860` and `test/js/node/process/call-raise.js` (same directory) already do.
Check failure on line 561 in src/js/builtins/ProcessObjectInternals.ts
claude / Claude Code Review
Windows defineProperty trap does not reject Symbol values
The Windows `defineProperty` trap checks `typeof p === "symbol"` for the key but not for `attributes.value`, so `Object.defineProperty(process.env, 'X', {value: Symbol(), writable:true, enumerable:true, configurable:true})` silently stores the string `"Symbol()"` instead of throwing TypeError — diverging from Node and from this PR's own POSIX `JSProcessEnvMap::defineOwnProperty` (which routes through `put()` → `value.isSymbol()`). Add `if (typeof attributes.value === "symbol") throw new TypeErro
Check warning on line 33 in test/js/node/process/process-env-exotic.test.ts
claude / Claude Code Review
process.env test cleanup runs after assertions instead of in try/finally
Several tests in the first `describe` block (and at lines 61-63, 67-70, 97-104, 115-126) set `process.env.ENVFIX_*` in the runner process and `delete` the keys after the `expect(...)` calls rather than in a `try { ... } finally { delete ... }`. With this PR a POSIX main-thread write now reaches real `libc::setenv()`, so a failed assertion leaks the var into `environ` for the rest of the file — REVIEW.md: "restore mutated globals in `finally`". Practical impact is low here (unique `ENVFIX_*` pref
Check warning on line 218 in test/js/node/process/process-env-exotic.test.ts
claude / Claude Code Review
execve launcher tests fail instead of skip when no C compiler is present
The `compile` helper does `expect(cc).toBeTruthy()`, which fails the test on a runner without a C compiler instead of skipping it. REVIEW.md: "Tests that need a system binary … `skipIf` when the dependency is unavailable." Hoist `const cc = Bun.which("clang") || Bun.which("gcc") || Bun.which("cc")` to module scope and change the describe to `describe.skipIf(!isPosix || !cc)`, matching the pattern in `test/js/bun/io/bun-write.test.js:548` and `test/regression/issue/30717.test.ts`.
Check warning on line 2256 in src/runtime/api/BunObject.rs
claude / Claude Code Review
Stale doc comment claims Bun__ProcessEnv__put returns a value
The doc comment on `Bun__ProcessEnv__put` says "Returns the env_loader map's entry count so the caller … can tell whether the JS enumeration view needs resizing", but the function returns `()` and the C++ extern is declared `void`. Looks like leftover text from an earlier iteration — the last sentence of the doc comment can be dropped.