fix: prevent setEnv from clobbering envConfig when merging NODE_CONFIG by sputnik-mac · Pull Request #896 · node-config/node-config

sputnik-mac · 2026-03-01T11:30:41Z

Problem

When building the combined NODE_CONFIG value from the environment variable and --NODE_CONFIG command-line argument, Util.extendDeep was called with envConfig as the target object:

// Before (buggy)
load.setEnv('NODE_CONFIG', JSON.stringify(Util.extendDeep(envConfig, cmdLineConfig, {})));

Since extendDeep mutates its first argument in-place, this clobbers envConfig itself — the config from the $NODE_CONFIG environment variable is overwritten before it can be used further.

Fix

Use an empty object {} as the merge target so neither envConfig nor cmdLineConfig is mutated:

// After (fixed)
load.setEnv('NODE_CONFIG', JSON.stringify(Util.extendDeep({}, envConfig, cmdLineConfig)));

This is consistent with the pattern used elsewhere in the codebase (e.g. Util.extendDeep({}, ...)).

Fixes #894

Signed-off-by: Sputnik-MAC [email protected]

Summary by CodeRabbit

Refactor
- Configuration merging now builds a fresh merged object so environment-derived config objects are not mutated during combine operations.
Tests
- Added regression tests ensuring command-line overrides take precedence, environment-only keys are preserved, and source objects remain immutable after merging.

When building the combined NODE_CONFIG value from the environment variable and --NODE_CONFIG command-line argument, Util.extendDeep was called with envConfig as the target object. This caused extendDeep to mutate envConfig in-place, effectively clobbering it. Fix by using an empty object {} as the merge target so neither envConfig nor cmdLineConfig is mutated. Fixes: node-config#894 Signed-off-by: Sputnik-MAC <[email protected]>

coderabbitai · 2026-03-01T11:31:00Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 26f01de and 8522025.

📒 Files selected for processing (1)

test/24-setenv-nodeconfig-merge.js

🚧 Files skipped from review as they are similar to previous changes (1)

test/24-setenv-nodeconfig-merge.js

📝 Walkthrough

Walkthrough

The merge target in config construction was changed to use a fresh object (Util.extendDeep({}, envConfig, cmdLineConfig)), avoiding mutation of the original envConfig. A regression test was added to verify merge behavior and immutability of the env source.

Changes

Cohort / File(s)	Summary
Configuration merge fix `lib/config.mjs`	Change merge call to `Util.extendDeep({}, envConfig, cmdLineConfig)` so merging does not mutate `envConfig`.
Merge behavior tests `test/24-setenv-nodeconfig-merge.js`	Add regression tests that assert CLI `NODE_CONFIG` overrides env `NODE_CONFIG`, preserve exclusive keys from both sources, and ensure the env source object is not mutated.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

bug

Poem

🐰 I hop through code with whiskers bright,
I tuck old objects safe from bite,
Merged fresh and tidy, nothing torn,
Configs intact at early morn,
A little hop, a cleaner sight!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately describes the main fix: preventing mutation of envConfig during NODE_CONFIG merging, which is the primary change in the PR.
Linked Issues check	✅ Passed	The PR addresses all coding requirements from issue `#894`: fixes the extendDeep call to use empty object as target and adds comprehensive regression tests validating merge semantics and non-mutation of source objects.
Out of Scope Changes check	✅ Passed	All changes are in scope: the config.mjs fix directly addresses the issue, the regression test file validates the fix, and the process state restoration in test hooks prevents side effects.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

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

coderabbitai

🧹 Nitpick comments (1)

lib/config.mjs (1)

697-697: Please add a regression test for this merge behavior.

Add a test that verifies: (1) source env config object is not mutated, and (2) --NODE_CONFIG overrides $NODE_CONFIG in the final NODE_CONFIG payload.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/config.mjs` at line 697, Add a regression test that exercises
load.setEnv('NODE_CONFIG', JSON.stringify(Util.extendDeep({}, envConfig,
cmdLineConfig))) to ensure merge behavior: create an original envConfig object
and a cmdLineConfig object, call the code path that performs Util.extendDeep({},
envConfig, cmdLineConfig) (or call load.setEnv with those values) and assert two
things—(1) the original envConfig object remains strictly equal to its pre-call
shape (not mutated), and (2) when both envConfig and cmdLineConfig set the same
key, the value from cmdLineConfig (simulating --NODE_CONFIG) appears in the
final NODE_CONFIG payload produced by load.setEnv /
JSON.parse(process.env.NODE_CONFIG). Use the actual symbols envConfig,
cmdLineConfig, Util.extendDeep and load.setEnv in the test to locate and
exercise the behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@lib/config.mjs`:
- Line 697: Add a regression test that exercises load.setEnv('NODE_CONFIG',
JSON.stringify(Util.extendDeep({}, envConfig, cmdLineConfig))) to ensure merge
behavior: create an original envConfig object and a cmdLineConfig object, call
the code path that performs Util.extendDeep({}, envConfig, cmdLineConfig) (or
call load.setEnv with those values) and assert two things—(1) the original
envConfig object remains strictly equal to its pre-call shape (not mutated), and
(2) when both envConfig and cmdLineConfig set the same key, the value from
cmdLineConfig (simulating --NODE_CONFIG) appears in the final NODE_CONFIG
payload produced by load.setEnv / JSON.parse(process.env.NODE_CONFIG). Use the
actual symbols envConfig, cmdLineConfig, Util.extendDeep and load.setEnv in the
test to locate and exercise the behavior.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c20341 and fcf9060.

📒 Files selected for processing (1)

lib/config.mjs

jdmarshall · 2026-03-01T16:31:33Z

I was hoping for unit tests but the only ones I can think of are to mock load.scan().

Verifies that: - --NODE_CONFIG overrides $NODE_CONFIG for shared keys - $NODE_CONFIG exclusive keys survive the merge - --NODE_CONFIG exclusive keys are present in merged result - the $NODE_CONFIG source object is not mutated by extendDeep (the original bug: cmd-only keys must not bleed into envConfig) Requested-by: CodeRabbit review on node-config#896 Signed-off-by: Sputnik-MAC <[email protected]>

sputnik-mac · 2026-03-01T16:35:20Z

Thanks for the review! I've added a regression test suite in test/24-setenv-nodeconfig-merge.js (commit 26f01de) that covers all four cases:

--NODE_CONFIG overrides $NODE_CONFIG for shared keys
$NODE_CONFIG-exclusive keys survive the merge
--NODE_CONFIG-exclusive keys appear in the merged result
The $NODE_CONFIG source object is not mutated by extendDeep — the core regression check (cmd-only keys must not bleed into envConfig)

All four tests pass against the fixed code.

coderabbitai

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/24-setenv-nodeconfig-merge.js`:
- Around line 19-36: The before hook mutates global process.env and process.argv
and doesn't restore them; capture originals at start of the suite (e.g., const
_origEnv = { ...process.env }; const _origArgv = process.argv.slice()) and add
an after hook that restores them (Object.assign(process.env, _origEnv) and
process.argv = _origArgv) and also delete any added keys (NODE_CONFIG_DIR,
NODE_ENV, NODE_CONFIG) or restore their previous values; ensure CONFIG is
cleared or reset if needed (e.g., delete from module cache or set CONFIG =
undefined) so the global process state is identical after the suite.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fcf9060 and 26f01de.

📒 Files selected for processing (1)

test/24-setenv-nodeconfig-merge.js

test/24-setenv-nodeconfig-merge.js

Add an after() hook that saves and restores process.argv, NODE_CONFIG_DIR, NODE_ENV, and NODE_CONFIG around the test suite, preventing state from leaking into subsequent test files. Signed-off-by: Sputnik-MAC <[email protected]>

coderabbitai bot reviewed Mar 1, 2026

View reviewed changes

test/24-setenv-nodeconfig-merge.js Show resolved Hide resolved

test: restore process state after setEnv regression suite

8522025

Add an after() hook that saves and restores process.argv, NODE_CONFIG_DIR, NODE_ENV, and NODE_CONFIG around the test suite, preventing state from leaking into subsequent test files. Signed-off-by: Sputnik-MAC <[email protected]>

jdmarshall mentioned this pull request Mar 2, 2026

fix: setEnv clobbering previous value (Issue #894) #897

Closed

jdmarshall approved these changes Mar 2, 2026

View reviewed changes

jdmarshall merged commit b0c9c9d into node-config:master Mar 2, 2026
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent setEnv from clobbering envConfig when merging NODE_CONFIG#896

fix: prevent setEnv from clobbering envConfig when merging NODE_CONFIG#896
jdmarshall merged 3 commits intonode-config:masterfrom
sputnik-mac:fix/setenv-clobbers-envconfig

sputnik-mac commented Mar 1, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Mar 1, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Suggested labels

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

jdmarshall commented Mar 1, 2026

Uh oh!

sputnik-mac commented Mar 1, 2026

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

sputnik-mac commented Mar 1, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested labels

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

jdmarshall commented Mar 1, 2026

Uh oh!

sputnik-mac commented Mar 1, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sputnik-mac commented Mar 1, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 1, 2026 •

edited

Loading