process/worker: env descriptor validation, worker execArgv policy table with per-worker --expose-gc (+3 tests, worker 74%→76%) - #34654
Code review found 2 important issues
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/jsc/bindings/JSEnvironmentVariableMap.cpp:611-616 |
preventExtensions override only on JSSharedEnvMap; missing on JSEnvironmentVariableMap and windowsEnv Proxy |
| 🟡 Nit | test/js/node/process/process.test.js:33-35 |
Stale test title: 'coerces accessor reads' but the test asserts accessors are rejected |
| 🟡 Nit | src/jsc/bindings/JSEnvironmentVariableMap.cpp:962-964 |
Redundant forward declaration of wrapInWindowsEnvProxy in the .cpp |
Annotations
Check failure on line 616 in src/jsc/bindings/JSEnvironmentVariableMap.cpp
claude / Claude Code Review
preventExtensions override only on JSSharedEnvMap; missing on JSEnvironmentVariableMap and windowsEnv Proxy
The `preventExtensions` override exists only on `JSSharedEnvMap` (line 613); it was dropped from the regular map when the main merge (3bb4d693, #31831) renamed `JSProcessEnvMap` → `JSEnvironmentVariableMap`, and the `windowsEnv` Proxy never got a `preventExtensions` trap — so the PR description's "Both env maps fail `[[PreventExtensions]]`" row is now false on the primary (main-thread) code path. On POSIX/Windows regular `process.env`, `Object.preventExtensions` succeeds and `Object.freeze` thro
Check warning on line 35 in test/js/node/process/process.test.js
claude / Claude Code Review
Stale test title: 'coerces accessor reads' but the test asserts accessors are rejected
The test title says "…and coerces accessor reads", but the body asserts the opposite: an accessor descriptor throws `ERR_INVALID_OBJECT_DEFINE_PROPERTY` (line 65: `accessor: "ERR_INVALID_OBJECT_DEFINE_PROPERTY"`), and the inline comment reads "Accessor descriptors are rejected like node's EnvDefiner". The 'coerces accessor reads' phrasing is a leftover from the 4f1552f0 revision — after the #31831 merge, the windowsEnv trap (ProcessObjectInternals.ts:558-561) rejects accessors outright. Nit only
Check warning on line 964 in src/jsc/bindings/JSEnvironmentVariableMap.cpp
claude / Claude Code Review
Redundant forward declaration of wrapInWindowsEnvProxy in the .cpp
Commit 846e5167 added `wrapInWindowsEnvProxy` to `JSEnvironmentVariableMap.h` (inside `namespace Bun`, gated on `OS(WINDOWS)`) so `ZigGlobalObject.cpp` can call it, but the .cpp still carries the earlier in-file forward declaration just above `createEnvironmentVariablesMap`. Since the .cpp includes its own header at line 5, this three-line `#if OS(WINDOWS)` block is now redundant. REVIEW.md: *"Delete dead code in the same PR that makes it dead."* Nit only (no runtime effect); delete the block.