dotenv: expose auto-loaded .env values as non-enumerable on process.env

[robobun]

What

Values that Bun auto-discovers in .env / .env.local / .env.{NODE_ENV} are now installed on process.env as non-enumerable accessor properties. Direct reads (process.env.FOO, 'FOO' in process.env, Object.hasOwn(process.env, 'FOO')) still work, but Object.keys(process.env), for..in, and { ...process.env } no longer list them.

--env-file values and OS env vars stay enumerable. Assigning to a conditional key from JS promotes it to an enumerable data property. Bun.spawn's default inherited environment (no env option) still includes .env values via the loader map.

Repro

d=$(mktemp -d); cd $d
printf 'PUBLICPATH=/\n'    > .env
printf 'PUBLICPATH=/app\n' > .env.production
cat > check.js <<'JS'
import { loadEnv } from 'vite';
console.log(loadEnv('production', process.cwd(), '').PUBLICPATH);
JS
bun add vite >/dev/null
node check.js   # /app
bun  check.js   # before: /   after: /app

Cause

Bun's auto-loaded .env values were indistinguishable from OS environment variables on process.env. Vite's loadEnv() (and dotenv-expand) follow the standard dotenv convention that existing process.env entries take priority over file values; both probe that via { ...process.env } and for (const key in process.env). When Bun runs without NODE_ENV set it defaults to the development file set, so .env lands in process.env.PUBLICPATH, and Vite then refuses to let .env.production override what it believes is a shell-provided value.

Fix

• HashTableValue regains a conditional: bool, set only by load_default_files (the auto-discovered .env* files). load_process, --env-file, and programmatic put() keep conditional = false.
• A new Bun__isEnvKeyConditional FFI exposes the flag to C++.
• createEnvironmentVariablesMap installs conditional keys as CustomAccessor | DontEnum with a non-caching getter and the existing jsSetterEnvironmentVariable so a JS write promotes to an enumerable data property. The Windows Proxy's key array skips conditional keys to keep ownKeys() consistent.
• Docs updated to describe the enumeration behaviour.

The --env-file describe block's "fallback to default dotenv behavior" test is updated to check via direct read instead of Object.entries; its intent (auto .env loading still happens without --env-file) is preserved.

Why this is correct

Under Node, process.env enumeration only ever yields OS environment variables, so any tool that treats enumerable process.env keys as "real env wins" works by construction. This change brings Bun's enumeration in line with that contract while keeping the ergonomic process.env.FOO read that Bun's auto-loading exists for. The earlier conditional field that #9689 removed was only consulted by the script-runner's subprocess env block; this revives it for the in-process process.env object instead, which is the layer the Vite conflict actually lives in.

Behavior change

Object.keys(process.env) / { ...process.env } no longer include values that came only from an auto-discovered .env* file. This is an observable change; see the updated docs in docs/runtime/environment-variables.mdx. Bun.spawn with the default (unspecified) env still inherits everything.

Verification

bun bd test test/cli/run/env.test.ts test/cli/run/no-envfile.test.ts
# 104 pass, 1 skip, 2 todo, 0 fail

Fixes #6338
Fixes #13614
Fixes #22496

---

no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/env.test.ts

[coderabbitai]

Walkthrough

Changes

Dotenv enumerability

Layer / File(s)	Summary
Conditional environment entry model src/dotenv/env_loader.rs, src/install/PackageManager.rs, src/install_jsc/ini_jsc.rs, src/runtime/cli/test_command.rs	HashTableValue entries now record whether values came from auto-discovered dotenv files; parser call sites and direct initializers populate this flag.
Process environment runtime semantics src/runtime/api/BunObject.rs, src/jsc/bindings/JSEnvironmentVariableMap.*, src/js/builtins/ProcessObjectInternals.ts	Conditional entries use non-caching, non-enumerable accessors, while Windows proxy writes and key tracking preserve enumeration after JavaScript assignment.
Worker propagation and behavior coverage src/jsc/bindings/webcore/JSWorker.cpp, test/cli/run/env.test.ts, docs/runtime/environment-variables.mdx	Worker snapshots include process environment values that are non-enumerable, and tests and documentation cover dotenv loading, enumeration, assignment, workers, and shared environments.

Possibly related PRs

• oven-sh/bun#34727: Both modify Windows process.env proxy descriptor handling.
• oven-sh/bun#35254: Both modify process.env accessor and enumerability behavior.
• oven-sh/bun#35270: Both modify environment-map handling for dotenv-origin keys.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address the reported Vite/Rsbuild priority issue by preserving mode-specific env loading while keeping auto-loaded .env values readable.
Out of Scope Changes check	✅ Passed	The touched docs, runtime, bindings, and tests all support the dotenv enumerability fix and its platform-specific behavior.
Title check	✅ Passed	The title clearly summarizes the main change: auto-loaded .env values are now non-enumerable on process.env.
Description check	✅ Passed	The description covers the behavior change and verification, though it uses custom headings instead of the exact template.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 2:13 AM PT - Jul 25th, 2026}

@robobun, your commit 7ef4f00 is building: #80455

[github-actions]

Found 4 issues this PR may fix:

1. Priority issue of environment variable modes when bundling the project with Bun #13614 - Vite's loadEnv enumerates process.env and treats Bun's pre-loaded .env values as system vars with highest priority, preventing mode-specific .env.prod files from overriding them
2. Vite env doesn't work. Import.meta.env #8548 - Bun pre-loads .env values as enumerable, so Vite treats them as system env vars and skips its own .env loading, leaving import.meta.env values undefined
3. env variables are not loading in bun 1.0.36 or 1.1 #9877 - Vite sees VITE_BACKEND in Object.keys(process.env) as a system env var and skips loading it through its own .env pipeline, so it never appears in import.meta.env
4. Environment variables loading incorrectly in rsbuild project when using bun run --bun build #22496 - rsbuild sees Bun's pre-loaded .env.development values as system env vars that take priority over .env.production during builds

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #13614
Fixes #8548
Fixes #9877
Fixes #22496

🤖 Generated with Claude Code

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/jsc/bindings/JSEnvironmentVariableMap.cpp:825 — This line makes jsSetterEnvironmentVariable the live [[Set]] path for every auto-loaded .env key (it was previously defined but never referenced — cached_getter_setter passes nullptr for the setter). Its body calls JSValue::decode(value).toString(globalObject) with no DECLARE_THROW_SCOPE and no RETURN_IF_EXCEPTION; on a throwing Symbol.toPrimitive/toString, JSC returns a non-null empty JSString* (not nullptr), so the if (!string) guard is bypassed and putDirect writes "" with an exception pending — a throwing assignment both throws and silently overwrites the .env value, and trips BUN_JSC_validateExceptionChecks=1. Add DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION(scope, false) after toString, matching the adjacent jsSetterProxyEnvironmentVariable.

  Extended reasoning...

  What the bug is

  Line 825 wires jsSetterEnvironmentVariable in as the setter half of conditional_getter_setter. At the base commit (028f7a3b), that function was defined but never referenced as a setter anywhere — cached_getter_setter passed nullptr for its setter, and the only other hit was a comment. It was dead code. This PR makes it the live [[Set]] path for every auto-loaded .env* key on POSIX (the property is installed with PropertyAttribute::CustomAccessor, so every write invokes the custom setter).

  Its body:

  VM& vm = globalObject->vm();
  JSC::JSObject* object = JSValue::decode(thisValue).getObject();
  if (!object) return false;
  auto string = JSValue::decode(value).toString(globalObject);   // can run user JS, can throw
  if (!string) [[unlikely]] return false;                          // toString() never returns null on exception
  object->putDirect(vm, propertyName, string, 0);
  return true;

  There is no DECLARE_THROW_SCOPE and no RETURN_IF_EXCEPTION after toString(globalObject). JSValue::toString invokes user Symbol.toPrimitive/toString/valueOf, which can throw; on exception JSC calls toStringSlowCase(..., returnEmptyStringOnError=true) and returns jsEmptyString(vm), not nullptr. So if (!string) does not catch it, and the code proceeds to putDirect with a pending exception — mutating process.env to the empty string as an observable side effect of a failed assignment — and returns true.

  Step-by-step proof

  On POSIX with .env containing API_KEY=secret (and nothing for that key in the OS environment):

  1. load_default_files → load_env_file::<false> → Parser::parse_bytes::<false, false, true, /*CONDITIONAL=*/true> writes HashTableValue { value: b"secret", conditional: true } for API_KEY.
  2. createEnvironmentVariablesMap calls Bun__isEnvKeyConditional → true → installs conditional_getter_setter with CustomAccessor | DontEnum for API_KEY.
  3. User code runs process.env.API_KEY = { toString() { throw new Error('boom'); } };.
  4. Because the property is a CustomAccessor, [[Set]] invokes jsSetterEnvironmentVariable.
  5. JSValue::decode(value).toString(globalObject) calls the user's toString(), which throws. toStringSlowCase catches the pending exception path and returns jsEmptyString(vm) — a valid, non-null JSString*.
  6. if (!string) is false; execution falls through.
  7. object->putDirect(vm, propertyName, string, 0) runs with an exception pending, replacing the DontEnum accessor with an enumerable data property whose value is the empty string.
  8. The setter returns true; the pending exception propagates to the caller.

  Under BUN_JSC_validateExceptionChecks=1 BUN_JSC_dumpSimulatedThrows=1 (which CLAUDE.md instructs contributors to run), this trips the exception-scope validator at the next scope. In release, the assignment throws to the caller and process.env.API_KEY has been silently overwritten with "" and promoted to an enumerable own property — so a subsequent catch-and-retry reads the wrong value, and Object.keys(process.env) now includes API_KEY even though the write conceptually failed.

  Why existing code doesn't prevent it

  The immediately-adjacent jsSetterProxyEnvironmentVariable in the same file shows the correct pattern: DECLARE_THROW_SCOPE(vm) at the top and RETURN_IF_EXCEPTION(scope, false) right after toString, followed by the if (!string) check — proving that the null guard alone is insufficient in this codebase's own idiom. There are ~88 occurrences of .toString(globalObject); RETURN_IF_EXCEPTION across src/jsc/bindings/. jsSetterEnvironmentVariable was never fixed because it was unreachable; this PR makes it reachable without adding the check.

  Windows is unaffected: the Proxy set trap coerces via String(value) in JS before writing to the underlying object, so the setter only ever sees a JSString and toString is a no-op there.

  Impact

  REVIEW.md lists this in the most-blocked category: "Exception checks after every call that can enter JS. Every call that can throw or run user code (toString/toNumber, ...) needs RETURN_IF_EXCEPTION under a ThrowScope (C++) ... before its result is used." This PR activates previously-dead code that violates that rule and produces observably wrong behavior (side-effect-on-error). It is not pre-existing — before this PR the function was unreferenced dead code.

  Fix

  Add a throw scope and check to jsSetterEnvironmentVariable, matching jsSetterProxyEnvironmentVariable:

  JSC_DEFINE_CUSTOM_SETTER(jsSetterEnvironmentVariable, ...)
  {
      VM& vm = globalObject->vm();
      auto scope = DECLARE_THROW_SCOPE(vm);
      JSC::JSObject* object = JSValue::decode(thisValue).getObject();
      if (!object)
          return false;
  
      auto* string = JSValue::decode(value).toString(globalObject);
      RETURN_IF_EXCEPTION(scope, false);
      if (!string) [[unlikely]]
          return false;
  
      object->putDirect(vm, propertyName, string, 0);
      return true;
  }

• 🔴 src/jsc/bindings/JSEnvironmentVariableMap.cpp:891-893 — Beyond the SHARE_ENV path flagged above, two more internal process.env enumeration sites now drop auto-loaded .env keys: (1) node:child_process's normalizeSpawnArguments (src/js/node/child_process.ts:1015-1057) builds the default child env via for (const key in env) over process.env and always sets [kBunEnv], so Bun.spawn's loader-map fallback is unreachable — child_process.spawn/exec/fork with no env option now silently drops .env-only values (same for node:cluster at primary.ts:81), while Bun.spawn({cmd}) with no env still inherits them; and (2) the non-SHARE_ENV worker_threads default path at JSWorker.cpp:284 snapshots the parent via getOwnPropertyNames(..., DontEnumPropertiesMode::Exclude) into options.env, and ZigGlobalObject.cpp then builds the worker's process.env as a plain object from that HashMap — bypassing createEnvironmentVariablesMap — so process.env.API_KEY inside the worker returns undefined, and whether the worker sees .env values non-deterministically depends on whether the parent touched process.env before spawning. Both are direct-read regressions that violate the PR's "direct reads still work" contract; per REVIEW.md "Fix the whole class in the same PR", all internal process.env enumeration sites need auditing alongside ensureSharedEnvStoreForWorker.

  Extended reasoning...

  What the bug is

  The comment above covers ensureSharedEnvStoreForWorker (the env: SHARE_ENV path). This PR's DontEnum change breaks two more internal consumers that materialise process.env by enumerating it, and neither is touched by the SHARE_ENV fix:

  (1) node:child_process default env inheritance

  normalizeSpawnArguments (src/js/node/child_process.ts:1015-1057) does:

  const env = options.env || process.env;
  const bunEnv = {};
  let envKeys = [];
  for (const key in env) { ArrayPrototypePush.$call(envKeys, key); }
  ...
  [kBunEnv]: bunEnv,   // always a truthy {}

  for..in skips the new CustomAccessor | DontEnum properties, so every key that came only from an auto-discovered .env* file is dropped from bunEnv. Because [kBunEnv] is set unconditionally to a truthy {}, both consumers pick it as the explicit env: for Bun.spawn:

  • spawnSync path — line 566: env: options[kBunEnv] || options.env || undefined
  • ChildProcess#spawn — line 1382: options[kBunEnv] || parseEnvPairs(envPairs) || process.env

  An explicit env object makes Bun.spawn set override_env = true (js_bun_spawn_bindings.rs), so the !override_env loader-map fallback that the PR description carves out ("Bun.spawn's default inherited environment … still includes .env values via the loader map") is unreachable from node:child_process.

  The same enumeration hits node:cluster at src/js/internal/cluster/primary.ts:81: { ...process.env, ...env, NODE_UNIQUE_ID: … } — spread also skips DontEnum.

  (2) worker_threads default (non-SHARE_ENV) env snapshot

  src/js/node/worker_threads.ts always passes an options object ({...options, preload:[…]}) to the native constructor. In constructJSWorker (src/jsc/bindings/webcore/JSWorker.cpp:271-297), when the user omits env:

  } else if (globalObject->m_processEnvObject.isInitialized()) {
      envObject = globalObject->processEnvObject();
  }
  ...
  envObject->methodTable()->getOwnPropertyNames(envObject, ..., JSC::DontEnumPropertiesMode::Exclude);  // :284
  ...
  options.env.emplace(WTF::move(env));  // :297

  The auto-loaded .env keys are filtered out of the snapshot. Then in ZigGlobalObject.cpp the worker sees options.env.has_value() → true and builds m_processEnvObject as a plain constructEmptyObject populated via putDirectMayBeIndex from that HashMap — bypassing createEnvironmentVariablesMap and the worker's cloned env-loader map. So inside the worker process.env.API_KEY returns undefined for any .env-only key: a direct-read failure, not just an enumeration change.

  Step-by-step proof

  child_process:

  // .env: DATABASE_URL=postgres://...
  require('child_process').execSync('printenv DATABASE_URL')

  1. normalizeSpawnArguments: options.env undefined → env = process.env.
  2. for (const key in env) — DATABASE_URL is DontEnum after this PR → skipped.
  3. bunEnv = {} (no DATABASE_URL); [kBunEnv]: bunEnv set.
  4. Line 566: env: options[kBunEnv] → {}-derived object passed to Bun.spawn.
  5. Bun.spawn sets override_env = true; loader-map default never consulted.
  6. Child (printenv, a non-Bun process) has no DATABASE_URL → exit 1.

  Before this PR the same key was an enumerable CustomValue accessor, so step 2 included it. Meanwhile Bun.spawn({cmd:['printenv','DATABASE_URL']}) with no env still prints the value via create_null_delimited_env_map() — so the two spawn APIs now silently diverge.

  worker_threads:

  // .env: API_KEY=secret
  process.env.PATH;                                // (a) reifies m_processEnvObject
  new Worker('data:text/javascript,console.log(process.env.API_KEY)');

  1. (a) initializes globalObject->m_processEnvObject.
  2. worker_threads.ts always passes {...options, preload:[…]}, so optionsObject is non-null and envValue = getIfPropertyExists("env") is empty.
  3. JSWorker.cpp:273 else if (m_processEnvObject.isInitialized()) → true → envObject = processEnvObject().
  4. JSWorker.cpp:284 enumerates with DontEnumPropertiesMode::Exclude → API_KEY (now DontEnum) is skipped.
  5. options.env.emplace(map) with only OS/enumerable keys.
  6. Worker init: options.env.has_value() → builds a plain JSObject from the HashMap; createEnvironmentVariablesMap and the cloned loader map are never consulted.
  7. Worker prints undefined.

  If the parent has not touched process.env before spawning, m_processEnvObject.isInitialized() is false, options.env stays nullopt, the worker falls through to createEnvironmentVariablesMap over its cloned loader map, and API_KEY is readable. So whether the worker sees .env values depends on whether the parent happened to read process.env first — spooky action at a distance.

  Why existing code doesn't prevent it

  • normalizeSpawnArguments unconditionally sets [kBunEnv] even when options.env was omitted, so the || undefined fallback at line 566 never fires and Bun.spawn's loader-map default is unreachable from node:child_process. No test under test/js/node/child_process/ exercises .env propagation.
  • The SHARE_ENV fix proposed in the previous comment (change ensureSharedEnvStoreForWorker to Include) does not touch JSWorker.cpp:284, which is a separate call site with a distinct symptom (the worker's process.env is a plain in-memory object with no fallback to the loader map — the value is unrecoverable inside the worker).

  Impact

  • child_process/cluster: silent behavior regression on a very common code path. child_process.execSync('migration-tool') from a Bun app that relies on .env for DATABASE_URL now runs the tool without it. If the child is a non-Bun process, or a Bun process launched with a different cwd, the value is gone entirely. This creates an undocumented Bun.spawn vs child_process.spawn inconsistency that neither the PR description nor the docs update mentions.
  • worker_threads: violates the PR's own "Direct reads (process.env.FOO …) still work" contract inside the worker, and makes .env visibility non-deterministic on unrelated parent-side reads.

  Per REVIEW.md "Fix the whole class in the same PR" (parallel switch arms, POSIX/Windows branches, every caller of a changed helper): these are sibling internal process.env enumeration sites of the same class as ensureSharedEnvStoreForWorker and should be fixed together.

  Fix

  • child_process/cluster: when options.env is not provided, leave [kBunEnv] unset (or set it to undefined) so the underlying Bun.spawn falls through to the native loader map — matching the PR's stated Bun.spawn contract. Alternatively, enumerate process.env with Object.getOwnPropertyNames (which includes non-enumerable own properties) when defaulting from process.env. If the divergence is intentional (child_process follows Node's OS-only inheritance), it must be documented alongside the enumeration change.
  • worker_threads default snapshot: either change JSWorker.cpp:284 to DontEnumPropertiesMode::Include (still filtering callables/undefined, as the SHARE_ENV seed does), or — when envValue is absent — leave options.env as nullopt regardless of m_processEnvObject.isInitialized() so the worker builds process.env via createEnvironmentVariablesMap over its cloned loader map (which already carries the correct conditional flags and thus preserves both direct reads and the new DontEnum semantics inside the worker).

[robobun]

All review concerns addressed across af13923, 24c4e37 and dff2fa0:

• SHARE_ENV store seeding now includes DontEnum keys (and unwraps the Windows Proxy to its target first) so founding a tree keeps auto-loaded .env values visible on the main thread.
• The default worker env snapshot in JSWorker.cpp does the same when the snapshot source is process.env; user-provided env objects still use the Exclude mode they always did.
• conditional is now honored for the special-cased TZ/TLS/fetch/proxy names and the integer-index path.
• The Windows Proxy set trap writes internalEnv[k] = value unconditionally so a same-value write still promotes a DontEnum key to enumerable, and defineProperty gates envMapList.push on list membership.
• The two sibling --env-file tests whose assertions had become vacuous now probe via direct access.

test/cli/run/env.test.ts and test/cli/run/no-envfile.test.ts pass on linux-x64 and windows-x64. CI running.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/jsc/bindings/JSEnvironmentVariableMap.cpp (1)

56-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared lookup body between the two env-var getters.

jsGetterConditionalEnvironmentVariable duplicates nearly all of jsGetterEnvironmentVariable (this-check, name extraction, Bun__getEnvValue call), differing only in whether the result is cached via putDirect. Since this is the code path that enforces the DontEnum/enumerability guarantee for auto-loaded .env values, keeping two near-identical implementations risks the two silently diverging on a future edit (e.g. an error-handling fix applied to one but not the other).

♻️ Suggested refactor: share the lookup, vary only the caching behavior

+static JSC::JSValue lookupEnvironmentVariable(JSGlobalObject* globalObject, JSC::EncodedJSValue thisValue, PropertyName propertyName, JSC::JSObject*& outThisObject)
+{
+    outThisObject = dynamicDowncast<JSObject>(JSValue::decode(thisValue));
+    if (!outThisObject) [[unlikely]]
+        return jsUndefined();
+
+    ZigString name = toZigString(propertyName.publicName());
+    ZigString value = { nullptr, 0 };
+    if (name.len == 0) [[unlikely]]
+        return jsUndefined();
+    if (!Bun__getEnvValue(globalObject, &name, &value))
+        return jsUndefined();
+    return jsString(globalObject->vm(), Zig::toStringCopy(value));
+}
+
 JSC_DEFINE_CUSTOM_GETTER(jsGetterEnvironmentVariable, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, PropertyName propertyName))
 {
-    ... (existing body) ...
+    JSC::JSObject* thisObject = nullptr;
+    JSValue result = lookupEnvironmentVariable(globalObject, thisValue, propertyName, thisObject);
+    if (thisObject && !result.isUndefined())
+        thisObject->putDirect(globalObject->vm(), propertyName, result, 0);
+    return JSValue::encode(result);
 }

 JSC_DEFINE_CUSTOM_GETTER(jsGetterConditionalEnvironmentVariable, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, PropertyName propertyName))
 {
-    ... (existing body) ...
+    JSC::JSObject* thisObject = nullptr;
+    return JSValue::encode(lookupEnvironmentVariable(globalObject, thisValue, propertyName, thisObject));
 }

Also applies to: 80-105

🤖 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 `@src/jsc/bindings/JSEnvironmentVariableMap.cpp` around lines 56 - 78, Extract
the common environment-variable lookup logic from jsGetterEnvironmentVariable
and jsGetterConditionalEnvironmentVariable into a shared helper covering
this-object validation, property-name extraction, empty-name handling, and
Bun__getEnvValue failure behavior. Keep the two getters responsible only for
their differing caching behavior, with jsGetterEnvironmentVariable retaining
putDirect and the conditional getter omitting it.

🤖 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.

Outside diff comments:
In `@src/jsc/bindings/JSEnvironmentVariableMap.cpp`:
- Around line 56-78: Extract the common environment-variable lookup logic from
jsGetterEnvironmentVariable and jsGetterConditionalEnvironmentVariable into a
shared helper covering this-object validation, property-name extraction,
empty-name handling, and Bun__getEnvValue failure behavior. Keep the two getters
responsible only for their differing caching behavior, with
jsGetterEnvironmentVariable retaining putDirect and the conditional getter
omitting it.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: af63a378-423f-49ba-b8f9-542db9ec73a8

📥 Commits

Reviewing files that changed from the base of the PR and between 27b2df9 and 8f71910.

📒 Files selected for processing (11)

• docs/runtime/environment-variables.mdx
• src/dotenv/env_loader.rs
• src/install/PackageManager.rs
• src/install_jsc/ini_jsc.rs
• src/js/builtins/ProcessObjectInternals.ts
• src/jsc/bindings/JSEnvironmentVariableMap.cpp
• src/jsc/bindings/JSEnvironmentVariableMap.h
• src/jsc/bindings/webcore/JSWorker.cpp
• src/runtime/api/BunObject.rs
• src/runtime/cli/test_command.rs
• test/cli/run/env.test.ts

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/jsc/bindings/JSEnvironmentVariableMap.cpp:724-728 — The DontEnumPropertiesMode::Include fix in af13923 is incomplete on both platforms. Windows: processEnvObject() is the Proxy, and ProxyObject::getOwnPropertyNames returns exactly the ownKeys() trap result (envMapList.slice()), which this PR filters conditional keys out of at line 843-846 — Include cannot add keys the trap omitted, so the store is seeded without them and process.env.AUTO_FROM_FILE reads undefined on the main thread after the swap (the new SHARE_ENV test will fail on Windows CI). POSIX: conditional keys are seeded, but JSSharedEnvMap::getOwnPropertyNames adds every store->keys() entry regardless of mode and getOwnPropertySlot returns attributes 0, so after new Worker(url, {env: SHARE_ENV}) the main thread's Object.keys(process.env) starts listing auto-loaded .env keys — contradicting the docs paragraph this PR adds. Fix: seed the store from the native env-loader map (Bun__getEnvCount/Bun__getEnvKey/Bun__isEnvKeyConditional) rather than the JS object, then overlay JS-enumerable own properties for user writes, and carry a per-key dontEnum bit through SharedEnvStore that JSSharedEnvMap::getOwnPropertyNames/getOwnPropertySlot honors and put clears.

  Extended reasoning...

  What the bug is

  Commit af13923 addressed the earlier SHARE_ENV review comment by switching ensureSharedEnvStoreForWorker's enumeration from DontEnumPropertiesMode::Exclude to ::Include (line 728), and adding a test that direct reads survive founding a SHARE_ENV tree. That fix has two remaining gaps — one per platform, in opposite directions.

  Windows: Include is a no-op through the Proxy — direct reads still lost

  On Windows, m_processEnvObject is the Proxy returned by processObjectInternalsWindowsEnvCodeGenerator (see createEnvironmentVariablesMap's profiledCall tail). Its ownKeys() trap (ProcessObjectInternals.ts:547-550) is:

  ownKeys() { return envMapList.slice(); }

  envMapList is the keyArray argument, and this PR gates keyArray->push on !conditional (line 843-846). So conditional keys are never in envMapList.

  ProxyObject::getOwnPropertyNames implements ES §10.5.11 [[OwnPropertyKeys]]: it invokes the ownKeys trap and returns exactly what the trap returned. DontEnumPropertiesMode::Include only means "do not post-filter the trap result by [[GetOwnProperty]].enumerable" — it cannot add keys the trap did not return. The ES invariant that would force target own-keys into the result only applies to non-configurable target keys; conditional accessors are installed as CustomAccessor | DontEnum with no DontDelete, so they are configurable and the invariant doesn't help. The target is extensible, so the extensibility invariant doesn't help either.

  POSIX: seeded keys become enumerable — docs contradiction

  On POSIX, envObject is a plain JSObject, so Include does return the DontEnum conditional accessors. jsGetterConditionalEnvironmentVariable returns the actual .env value (not undefined), so it passes the isUndefined()/isCallable() filters and store->set(key, str) records it.

  But SharedEnvStore (SharedEnvStore.h) is a plain String → String map with no per-key attribute bit. After the swap to JSSharedEnvMap:

  • getOwnPropertyNames does for (const auto& key : store->keys()) propertyNames.add(...) unconditionally — the DontEnumPropertiesMode mode parameter is ignored for store entries.
  • getOwnPropertySlot does slot.setValue(object, 0, jsString(vm, value)) — attributes 0 means enumerable: true.

  Step-by-step proof

  Windows — .env contains AUTO_FROM_FILE=secret, not in the OS env:

  1. createEnvironmentVariablesMap: conditional == true → keyArray->push skipped; internalEnv gets the key as CustomAccessor | DontEnum. Returns the Proxy.
  2. console.log(process.env.AUTO_FROM_FILE) → get trap → internalEnv[k.toUpperCase()] → conditional getter → "secret". ✓
  3. new Worker('./worker.js', { env: SHARE_ENV }) → ensureSharedEnvStoreForWorker:
     • envObject = processEnvObject() → the Proxy.
     • getOwnPropertyNames(envObject, ..., Include) → ProxyObject dispatches to ownKeys() trap → envMapList.slice() → AUTO_FROM_FILE absent.
     • Store seeded without it; m_processEnvObject swapped to JSSharedEnvMap.
  4. process.env.AUTO_FROM_FILE → JSSharedEnvMap::getOwnPropertySlot → store->get("AUTO_FROM_FILE") is null → falls to empty Base → undefined. ✗

  The new test at env.test.ts:314-330 expects "secret\nsecret" and will fail on Windows CI.

  POSIX — .env contains API_KEY=secret, not in the OS env:

  1. Before new Worker: Object.keys(process.env).includes('API_KEY') → false (DontEnum CustomAccessor). ✓
  2. new Worker(url, { env: SHARE_ENV }) → seed loop reads API_KEY via Include → getter returns "secret" → store->set("API_KEY", "secret") → main's process.env swapped to JSSharedEnvMap.
  3. After: Object.keys(process.env).includes('API_KEY') → getOwnPropertyNames iterates store->keys() → includes API_KEY; getOwnPropertySlot reports attributes 0 → true. ✗

  Same expression flips false → true on the main thread as a side effect of spawning a worker — action at a distance that directly contradicts the new docs section ("Values that Bun auto-loads from .env files … are not enumerable").

  Why nothing prevents it

  The comment at line 724-726 says "Include DontEnum so auto-loaded .env values … are seeded", which assumes the enumeration source is the object's own-property table. On Windows the source is the trap's return value, which line 843-846 already filtered. On POSIX the seed works, but SharedEnvStore has no per-key attribute storage and JSSharedEnvMap never consults Bun__isEnvKeyConditional. The new SHARE_ENV test only asserts direct reads (process.env.AUTO_FROM_FILE), not that Object.keys still excludes it after the swap, so the POSIX enumeration leak has no coverage.

  The earlier review comment's extended reasoning noted "On Windows the same loss occurs via a different route: the Proxy's ownKeys() trap returns keyArray, which this PR now builds by skipping conditional keys" — the applied fix took the Include suggestion, which only addresses the POSIX arm.

  Impact

  • Windows: The PR's core "direct reads still work" contract is still broken. Any app that both reads auto-loaded .env values and spawns a SHARE_ENV worker loses those values on the main thread the moment the first worker is created. The newly-added test will fail on Windows CI.
  • POSIX: The PR's core "not enumerable" contract is broken after founding a SHARE_ENV tree. If a build tool spawns a SHARE_ENV worker before running its own .env.{mode} loader, { ...process.env } re-includes the auto-loaded value and the original #6338 shadowing bug reappears on the main thread.

  Per REVIEW.md's "Fix the whole class in the same PR" (POSIX/Windows branches, fast/slow paths), and since af13923 already touched exactly this code path, both arms are in scope.

  Fix

  Seed the store from the source that actually knows the conditional bit, and carry that bit through:

  1. In ensureSharedEnvStoreForWorker, seed from the native env-loader map directly — iterate Bun__getEnvCount/Bun__getEnvKey/Bun__getEnvValue/Bun__isEnvKeyConditional (the same source createEnvironmentVariablesMap uses) — then overlay with the JS object's enumerable own properties to pick up user writes made since startup. This bypasses the Proxy ownKeys() trap on Windows and works identically on POSIX. (Alternatively, on Windows unwrap to the Proxy target — internalEnv — before enumerating with Include; but that doesn't solve the POSIX enumeration side, so you'd still need step 2.)
  2. Add a per-key dontEnum bit to SharedEnvStore (e.g. HashMap<String, std::pair<String, bool>>), set at seed time from the source's conditional flag. JSSharedEnvMap::getOwnPropertyNames skips those keys when mode == DontEnumPropertiesMode::Exclude; getOwnPropertySlot returns PropertyAttribute::DontEnum for them; JSSharedEnvMap::put/defineOwnProperty clear the bit (matching the "assigning promotes to enumerable" contract).
  3. Extend the SHARE_ENV test to also assert Object.keys(process.env).includes('AUTO_FROM_FILE') === false both before and after the worker is created.

• 🔴 src/jsc/bindings/JSEnvironmentVariableMap.cpp:901-904 — Installing conditional keys as DontEnum breaks three more process.env-enumerating consumers that build subprocess/worker env by default. node:child_process (normalizeSpawnArguments, child_process.ts:1015-1057) does for (const key in (options.env || process.env)) and always sets options[kBunEnv], so Bun.spawn never hits its loader-map fallback — execSync('printenv DATABASE_URL') with DATABASE_URL only in .env now fails. node:cluster (primary.ts:81) uses { ...process.env }. worker_threads default (JSWorker.cpp:273-297) enumerates the parent's process.env with DontEnumPropertiesMode::Exclude when no env: is passed, but only if m_processEnvObject.isInitialized() — so whether a default worker sees .env values now depends on whether the parent evaluated process.env first (before this PR both branches converged). All three contradict the PR's "direct reads still work" contract inside the child and diverge from the Bun.spawn no-env behavior the PR/docs explicitly preserve; same class as the SHARE_ENV site already fixed at line 728.

  Extended reasoning...

  What the bug is

  This PR installs auto-loaded .env keys on process.env with PropertyAttribute::DontEnum. The SHARE_ENV seed path (ensureSharedEnvStoreForWorker, line 728) was already updated to enumerate with DontEnumPropertiesMode::Include, but three sibling sites that build a child/worker environment by enumerating process.env were not:

  1. src/js/node/child_process.ts:1015-1057 — normalizeSpawnArguments does const env = options.env || process.env then for (const key in env) to build bunEnv, and unconditionally sets options[kBunEnv] = bunEnv. At line 566 (spawnSync) and line 1382 (ChildProcess#spawn), env: options[kBunEnv] || options.env || undefined — bunEnv is always a truthy object (it contains the OS env vars), so Bun.spawn receives an explicit env and never reaches the loader-map fallback the PR description relies on. Every child_process.{spawn,exec,execFile,fork,spawnSync,execSync,execFileSync} call without options.env now drops .env-only values.

  2. src/js/internal/cluster/primary.ts:81 — createWorkerProcess builds workerEnv = { ...process.env, ...env, NODE_UNIQUE_ID: \${id}` }. Spread skips DontEnum, so cluster workers' explicit env` omits auto-loaded values.

  3. src/jsc/bindings/webcore/JSWorker.cpp:273-297 — for new Worker(url) with no env: option and no SHARE_ENV, the code checks else if (globalObject->m_processEnvObject.isInitialized()) and, if the parent's lazy process.env has fired, enumerates it with DontEnumPropertiesMode::Exclude (line 284), then options.env.emplace(...) (line 297). ZigGlobalObject.cpp:571-590 materializes that as a plain JSFinalObject via putDirectMayBeIndex — no accessors, no env-loader fallback — so process.env.API_KEY === undefined in the worker.

  The Heisenbug (site 3)

  Whether a default new Worker(url) sees .env values now depends on whether the parent already evaluated process.env:

  • Parent never touched process.env → m_processEnvObject.isInitialized() is false → envObject stays null → options.env not emplaced → worker falls through to m_processEnvObject.initLater → createEnvironmentVariablesMap over its cloned env-loader map (Map::clone_with_allocator() copies HashTableValue including conditional) → worker sees .env values.
  • Parent read process.env.ANYTHING first → lazy init fired → Exclude enumeration skips DontEnum conditional keys → worker's process.env is a plain object without them → process.env.API_KEY === undefined.

  Before this PR the .env keys were enumerable CustomValue accessors, so Exclude returned them and both branches converged.

  Step-by-step proof

  child_process — with .env containing DATABASE_URL=postgres://x and nothing for it in the OS environment:

  1. load_default_files → Parser::parse_bytes::<false, false, true, /* CONDITIONAL = */ true> sets conditional = true on DATABASE_URL.
  2. createEnvironmentVariablesMap: Bun__isEnvKeyConditional → true → installed as CustomAccessor | DontEnum.
  3. User calls require('child_process').execSync('printenv DATABASE_URL') with no env option.
  4. normalizeSpawnArguments: env = options.env || process.env → process.env; for (const key in env) skips DATABASE_URL (DontEnum); bunEnv is built with only OS env vars; options[kBunEnv] = bunEnv.
  5. spawnSync → Bun.spawnSync({ env: options[kBunEnv] || ... }) — bunEnv is truthy, so it's passed as the explicit env. The loader-map fallback in Bun.spawn (which the PR description says still includes .env values) is never reached.
  6. Child's OS env has no DATABASE_URL; printenv exits nonzero. Before this PR: printed postgres://x.

  worker_threads — with .env containing API_KEY=secret:

  const { Worker } = require('worker_threads');
  console.log(process.env.API_KEY);  // 'secret' — this ALSO forces m_processEnvObject init
  new Worker(`data:text/javascript,console.log(process.env.API_KEY)`);
  // worker prints: undefined     (regression; before this PR: 'secret')

  Delete the parent's console.log line and the worker prints secret again — order-dependent behavior on unrelated parent-thread code.

  Why nothing prevents it

  The PR description explicitly promises "Bun.spawn's default inherited environment (no env option) still includes .env values via the loader map", but node:child_process never reaches that fallback because normalizeSpawnArguments unconditionally materializes bunEnv even when options.env was undefined. The JSWorker.cpp site is the direct native sibling of the SHARE_ENV seed path that was fixed, but it was left at Exclude. No test in this PR covers child_process/cluster/default-worker inheritance of a .env-only value. Per REVIEW.md "Fix the whole class in the same PR — grep for every sibling site sharing the pattern (sync/async twins, sibling entry points)": these enumeration sites are the same class as the SHARE_ENV site and now silently diverge from the Bun.spawn behavior the PR docs describe.

  Impact

  Silent behavioral regression on common paths: any Bun app that auto-loads .env and calls spawn('psql', ...), exec('python ...'), cluster.fork(), or new Worker(url) without an explicit env will stop passing .env values through. The worker case additionally introduces lazy-init-order-dependent behavior that neither the docs nor the PR description acknowledge. The PR's own docs update ("Direct reads still work") is contradicted inside default workers.

  Fix

  • child_process: when options.env was undefined, either skip building bunEnv entirely (leave options[kBunEnv] unset so Bun.spawn hits its loader-map default), or build it from Object.getOwnPropertyNames(process.env) filtered to defined string values.
  • cluster: same — seed workerEnv from Object.getOwnPropertyNames(process.env) (or leave env unset and only add NODE_UNIQUE_ID).
  • JSWorker.cpp default path: switch line 284 to DontEnumPropertiesMode::Include with the same isUndefined()/isCallable() filtering used in ensureSharedEnvStoreForWorker, or seed the worker directly from the native env-loader map when the user did not pass env: (mirroring Bun.spawn's no-env fallback).

[robobun]

Follow-up in d667f35 after second-round review:

• Reverted the Windows Proxy-target unwrap. Conditional keys are now kept in envMapList (the Proxy's ownKeys() result) and the getOwnPropertyDescriptor trap reports enumerable: false for them from internalEnv's DontEnum accessor, so Object.keys/for..in/spread still filter them out while Object.getOwnPropertyNames and DontEnumPropertiesMode::Include see them in the original case. That removes the key-casing regression and the need for any unwrap.
• ensureSharedEnvStoreForWorker and the default worker snapshot enumerate with Include directly on the Proxy (original case) and skip undefined/callable values.
• node:child_process now enumerates process.env via Object.getOwnPropertyNames when no options.env is given so children keep inheriting both auto-loaded .env values and runtime mutations. User-provided env objects still go through for..in.
• The Windows inspect/toJSON helpers now filter envMapList by the storage property's enumerability so their output matches POSIX.

test/cli/run/env.test.ts + test/cli/run/no-envfile.test.ts: 108/0 on linux-x64, 105/0 on windows-x64. Manual check on Windows confirms Path stays Path after founding a SHARE_ENV tree.

[claude]

No new issues found in this round. This is a user-facing behavior change to process.env enumeration semantics (auto-loaded .env values become non-enumerable), with compensating changes across eight default-env-inherit call sites, the Windows Proxy, and worker/SHARE_ENV snapshotting — worth a maintainer sign-off on the API decision itself.

What was reviewed:

• The conditional flag flow: only load_default_files sets it; --env-file, load_process, and all put() sites stay false.
• createEnvironmentVariablesMap's CustomAccessor|DontEnum arm and the non-caching getter; write-promotes-to-enumerable via jsSetterEnvironmentVariable.
• The Windows Proxy: envMapList now includes conditional keys (ownKeys sees them), enumerableView filters by descriptor, set/defineProperty traps gate on list membership.
• Each getOwnPropertyNames/Include compensated site (child_process, shell ×3, cluster, node:test, wasi, JSWorker, SHARE_ENV seeding) has a matching test except the two the author explicitly deferred (node:test run(), WASI).

Extended reasoning...

Overview

This PR changes process.env enumeration semantics: values Bun auto-discovers from .env/.env.local/.env.{NODE_ENV} are now installed as DontEnum CustomAccessors so Object.keys, for..in, and spread no longer list them (fixing Vite loadEnv and similar tools that treat enumerable process.env keys as shell-provided overrides). It touches 16 files: the Rust env loader (HashTableValue.conditional + a new CONDITIONAL const generic on the parser), a new Bun__isEnvKeyConditional FFI export, JSEnvironmentVariableMap.cpp (a non-caching conditional getter, per-key DontEnum branching, SHARE_ENV seeding switched to Include), JSWorker.cpp (env snapshot switched to Include when the object is process.env), the Windows process.env Proxy in ProcessObjectInternals.ts, and eight JS-side default-env-inherit call sites (child_process, Bun.$ ×3, cluster.fork, node:test run(), WASI runner) rewritten to snapshot via getOwnPropertyNames. Docs updated in two files; ~220 lines of tests added to env.test.ts.

Security risks

None identified. Environment values are not more exposed than before — they become less visible via enumeration. The compensated inherit paths filter undefined/callable values, so the always-present TZ/TLS/proxy accessors and Windows toJSON don't leak into child env blocks.

Level of scrutiny

High. This is an intentional, documented behavior change to a core runtime object that many tools introspect. The mechanism is sound and the implementation went through several review rounds here (Windows Proxy same-value-write promotion, defineProperty list gating, sibling call-site coverage, SHARE_ENV seeding, docs consistency), but the decision to make auto-loaded .env values non-enumerable — and the ripple that any code doing {...process.env} now silently drops them — is an API-design call that a maintainer should ratify, not a bot.

Other factors

• The author explicitly deferred three items to follow-ups: (1) TZ/NODE_TLS_REJECT_UNAUTHORIZED/BUN_CONFIG_VERBOSE_FETCH setters do not clear DontEnum on write (pre-existing gap now more visible), (2) a node:test run() default-env test, (3) a WASI environ test. All were acknowledged in resolved threads.
• Three existing --env-file tests were rewritten from Object.entries-based enumeration to direct-read assertions; the intent is preserved but the assertion surface changed.
• CI build #80263 is running against the head commit; I did not verify its status.
• The Windows-specific Proxy changes (push vs putByIndexInline, enumerableView, list-membership gating) are the trickiest platform-gated part and only exercised on Windows CI.

[claude]

No issues found in this pass, but this is a deliberate user-facing behavior change to process.env enumeration semantics across 16 files (Rust env loader, JSC C++ bindings, the Windows Proxy, and eight default-env-inheritance call sites), so a maintainer should sign off on the design.

What was reviewed:

• The conditional flag plumbing through env_loader.rs and its FFI export — load_default_files is the only path setting it true; --env-file, load_process, and put() all set false.
• createEnvironmentVariablesMap's DontEnum CustomAccessor arm and the write-promotes-to-enumerable path via jsSetterEnvironmentVariable; the Windows keyArray now uses push so ownKeys still lists conditional keys while the descriptor trap keeps them non-enumerable.
• The eight compensated default-inherit sites (child_process, Bun.$, cluster.fork, worker snapshot, SHARE_ENV seeding, node:test run, WASI runner) — each uses getOwnPropertyNames/Include and filters undefined/callable so unset TZ/proxy accessors and toJSON don't leak.
• The three modified --env-file describe tests were checked to still assert the same intent (auto-load fallback vs. disabled) via direct reads.

Extended reasoning...

Overview

This PR changes how Bun's auto-discovered .env* values appear on process.env: they become non-enumerable CustomAccessor | DontEnum properties so Object.keys / for..in / spread match Node's OS-only view, fixing tools like Vite's loadEnv that treat enumerable process.env entries as shell-provided overrides. It touches the Rust env loader (HashTableValue.conditional + a new const generic on Parser), a new Bun__isEnvKeyConditional FFI, JSEnvironmentVariableMap.cpp (a second non-caching getter, DontEnum installation, Windows keyArray handling, SHARE_ENV seeding via Include), JSWorker.cpp (isProcessEnv-gated Include snapshot), the Windows process.env Proxy in ProcessObjectInternals.ts, and six built-in JS modules that previously enumerated process.env via spread/for-in for default env inheritance. Docs and ~220 lines of new tests in env.test.ts round it out.

Security risks

None identified. The change narrows what enumerates out of process.env, not what is readable; direct reads and default subprocess/worker inheritance are preserved. No new untrusted-input parsing, no auth/crypto/permissions surface.

Level of scrutiny

High — this is an intentional, documented change to observable process.env semantics that every Bun user and a large fraction of the npm ecosystem interact with. It has cross-platform branches (the Windows Proxy path is materially different), and getting default-env inheritance wrong at any of the eight compensated call sites would silently drop .env values from child processes/workers. The implementation looks correct and well-tested after several review rounds, but the decision to ship this behavior (and its interaction with Bun.spawn({env: process.env}), which the author explicitly chose not to special-case) is a maintainer-level call.

Other factors

The PR has been through four review rounds; every prior finding is resolved, including the docs consistency fix in 5090e3d. Two hunks (WASI runner, node:test run()) were explicitly deferred by the author for follow-up test coverage — both are the same four-line getOwnPropertyNames snapshot used at the six other tested sites, so the risk is low, but it's another reason a human should confirm the deferral is acceptable. The author also deferred the pre-existing TZ/TLS/verbose-fetch setter DontEnum-clearing gap to a follow-up. Given the scope, the observable behavior change, and the deferred items, this warrants a human approval rather than a bot one.

[robobun]

CI on 7ef4f00 is green for everything this diff touches. The two remaining red lanes are known Windows flakes unrelated to this change:

• test/js/node/test/parallel/test-repl-close.js (EPIPE on write, passed on retry)
• test/cli/run/cpu-prof.test.ts ("No samples collected" on Windows, passed on retry)

test/cli/run/env.test.ts and test/cli/run/no-envfile.test.ts pass on every lane. Ready for maintainer review of the behavior change.

[robobun]

Closing: #6338, #13614 and #22496 were closed by #36610, which stops auto-loading .env files when Bun is invoked as node (the path the Vite and rsbuild reports go through). That approach landed instead of the non-enumerable process.env change proposed here, so this PR is superseded by #36610.