fix(runtime): make Object.assign(process.env, …) actually set environment variables#6404
Conversation
…ment variables
`process.env` is not an ordinary object — reads resolve through the runtime's env
lookup — but `Object.assign` treated it as one. The merged keys landed in a plain
object that nothing ever consulted, so every subsequent `process.env.X` read came
back `undefined`:
Object.assign(process.env, { DATABASE_URL: "mysql://…" });
process.env.DATABASE_URL; // node: "mysql://…" perry: undefined
`js_object_assign_one` now recognizes a `process.env` target and routes each
source key through `js_setenv`, so the values land where reads look for them. A
plain-object target is untouched. `js_setenv` additionally writes the key into
the cached env object, so a read that hits the cache sees it too.
This is how `@next/env` installs a parsed `.env` file (`Object.assign(process.env,
parsed)`), and how dotenv and most config loaders do it — so under Perry a Next.js
app silently ran with no environment at all: no `DATABASE_URL`, no API keys.
Found while compiling a real Next.js + MySQL app. Covered by
`test_gap_object_assign_process_env` — direct/bracket/dynamic-key reads, `in`,
overwrite, multi-source assign, plain assignment, and an ordinary-object target.
Byte-identical to node.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
Changesprocess.env assignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Source
participant ObjectAssign
participant js_setenv
participant CachedProcessEnv
Source->>ObjectAssign: Provide enumerable keys and values
ObjectAssign->>js_setenv: Set each process.env key/value
js_setenv->>CachedProcessEnv: Update cached property
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/object/alloc.rs`:
- Around line 1115-1142: Remove the early process.env fast-path block from
Object.assign and integrate the target check into object_assign_set_string_key.
When the target is the process.env object, route each property write through
crate::process::js_setenv; otherwise retain the existing property-assignment
behavior. Leave the existing source-decoding flow responsible for primitives,
arrays, proxies, and nullish sources.
In `@crates/perry-runtime/src/process/env_misc.rs`:
- Around line 886-898: Validate the environment variable name in the js_setenv
path before calling std::env::set_var, rejecting empty names and names
containing '=' or '\0'. For invalid keys, either silently ignore the assignment
or raise a JavaScript exception, but never invoke std::env::set_var; preserve
the existing cached process.env update only for valid names.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f08a426-a172-4764-9d29-4bf6ec433c19
📒 Files selected for processing (4)
crates/perry-runtime/src/object/alloc.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/process/env_misc.rstest-files/test_gap_object_assign_process_env.ts
…he entry
The env fast path sat at the top of js_object_assign_one and re-implemented
source decoding. That got three things wrong, all reachable from the very call
this PR exists to support -- Object.assign(process.env, parsed):
* It cast ANY source pointer to `*const ObjectHeader`. A string or array
source (`Object.assign(process.env, "abc")`) was then read through the
wrong layout -- type confusion.
* It enumerated the source with `js_object_keys_value`, which THROWS
ToObject's TypeError on null/undefined. The spec says a nullish source is
silently skipped, and node does; perry threw.
* It fed arbitrary object keys to `js_setenv` -> `std::env::set_var`, which
PANICS on a name that is empty or contains '=' or NUL. From an extern "C"
frame that panic cannot unwind, so it ABORTED the process (SIGABRT). One
malformed line in a .env file would have taken a Next.js server down --
exactly the workload this PR targets.
Move the check to `object_assign_set_string_key`, the single write funnel all
four call sites already go through. Every source shape now flows through the
existing decoding (primitives, arrays, proxies, nullish), and only the write is
redirected. Also validate the name in `js_setenv` and skip an unsettable one
rather than abort -- node accepts such an assignment silently.
Verified against node: the .env round-trip this PR fixes still works, and
nullish / string / empty-key / '='-key sources are now byte-identical instead of
throwing or aborting. Extended the gap test to cover all four.
|
CodeRabbit was right on both counts, and the impact is worse than reported. Fixed in I built the branch and ran the edge cases against node:
The bad-key case is not a catchable error — That is reachable from the exact workload this PR exists to support: The fix takes CodeRabbit's suggested architecture. The root problem was placing the env check as an early exit in Moved the check to Verified: the Skipping the third nitpick (thread-local |
1 participant
The bug
process.envis not an ordinary object — reads resolve through the runtime's env lookup — butObject.assigntreated it as one. The merged keys landed in a plain object that nothing ever consulted, so every subsequent read came backundefined:Why it matters
This is exactly how
@next/envinstalls a parsed.envfile (Object.assign(process.env, parsed)), and how dotenv and most config loaders do it. Under Perry a Next.js app therefore ran with no environment at all — noDATABASE_URL, no API keys — and the failure was silent: every variable simply read asundefined, far from the assignment that was supposed to set it.The fix
js_object_assign_onenow recognizes aprocess.envtarget and routes each source key throughjs_setenv, so the values land where reads look for them. A plain-object target is untouched.js_setenvadditionally writes the key into the cached env object, so a read that hits the cache sees it too.Test
test_gap_object_assign_process_env— direct / bracket / dynamic-key reads, theinoperator, overwriting an existing key, multi-source assign, plainprocess.env.X = …assignment, and an ordinary-object target (which must be unaffected). Byte-identical to node.Found while compiling a real Next.js + MySQL app.
Summary by CodeRabbit
Object.assign(process.env, values)so new keys are written correctly and become immediately readable.inoperator for assigned.env-style keys.Object.assignbehavior unchanged for non-process.envtargets (including returning the original target).process.env.