From 8c28250bb3e6982e1ecff907d9b6c365c6e371d7 Mon Sep 17 00:00:00 2001
From: jerop <jerop@google.com>
Date: Fri, 6 Jun 2025 15:32:39 +0000
Subject: [PATCH] Refactor: Improve env var resolution in settings

Refactors the `resolveEnvVarsInObject` function in settings to
explicitly handle primitive types (null, undefined, boolean, number)
at the beginning of the function. This clarifies the logic for
subsequent string, array, and object processing.
---
 packages/cli/src/config/settings.test.ts | 52 ++++++++++++++++++++++++
 packages/cli/src/config/settings.ts      | 11 ++++-
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts
index 350c5d33..a562c664 100644
--- a/packages/cli/src/config/settings.test.ts
+++ b/packages/cli/src/config/settings.test.ts
@@ -482,6 +482,57 @@ describe('Settings Loading and Merging', () => {
       delete process.env.ITEM_1;
       delete process.env.ITEM_2;
     });
+
+    it('should correctly pass through null, boolean, and number types, and handle undefined properties', () => {
+      process.env.MY_ENV_STRING = 'env_string_value';
+      process.env.MY_ENV_STRING_NESTED = 'env_string_nested_value';
+
+      const userSettingsContent = {
+        nullVal: null,
+        trueVal: true,
+        falseVal: false,
+        numberVal: 123.45,
+        stringVal: '$MY_ENV_STRING',
+        nestedObj: {
+          nestedNull: null,
+          nestedBool: true,
+          nestedNum: 0,
+          nestedString: 'literal',
+          anotherEnv: '${MY_ENV_STRING_NESTED}',
+        },
+      };
+
+      (mockFsExistsSync as Mock).mockImplementation(
+        (p: fs.PathLike) => p === USER_SETTINGS_PATH,
+      );
+      (fs.readFileSync as Mock).mockImplementation(
+        (p: fs.PathOrFileDescriptor) => {
+          if (p === USER_SETTINGS_PATH)
+            return JSON.stringify(userSettingsContent);
+          return '{}';
+        },
+      );
+
+      const settings = loadSettings(MOCK_WORKSPACE_DIR);
+
+      expect(settings.user.settings.nullVal).toBeNull();
+      expect(settings.user.settings.trueVal).toBe(true);
+      expect(settings.user.settings.falseVal).toBe(false);
+      expect(settings.user.settings.numberVal).toBe(123.45);
+      expect(settings.user.settings.stringVal).toBe('env_string_value');
+      expect(settings.user.settings.undefinedVal).toBeUndefined();
+
+      expect(settings.user.settings.nestedObj.nestedNull).toBeNull();
+      expect(settings.user.settings.nestedObj.nestedBool).toBe(true);
+      expect(settings.user.settings.nestedObj.nestedNum).toBe(0);
+      expect(settings.user.settings.nestedObj.nestedString).toBe('literal');
+      expect(settings.user.settings.nestedObj.anotherEnv).toBe(
+        'env_string_nested_value',
+      );
+
+      delete process.env.MY_ENV_STRING;
+      delete process.env.MY_ENV_STRING_NESTED;
+    });
   });
 
   describe('LoadedSettings class', () => {
@@ -518,6 +569,7 @@ describe('Settings Loading and Merging', () => {
       );
 
       loadedSettings.setValue(SettingScope.Workspace, 'theme', 'ocean');
+
       expect(loadedSettings.workspace.settings.theme).toBe('ocean');
       expect(loadedSettings.merged.theme).toBe('ocean');
     });
diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts
index 8205a018..427fb898 100644
--- a/packages/cli/src/config/settings.ts
+++ b/packages/cli/src/config/settings.ts
@@ -110,6 +110,15 @@ function resolveEnvVarsInString(value: string): string {
 }
 
 function resolveEnvVarsInObject<T>(obj: T): T {
+  if (
+    obj === null ||
+    obj === undefined ||
+    typeof obj === 'boolean' ||
+    typeof obj === 'number'
+  ) {
+    return obj;
+  }
+
   if (typeof obj === 'string') {
     return resolveEnvVarsInString(obj) as unknown as T;
   }
@@ -118,7 +127,7 @@ function resolveEnvVarsInObject<T>(obj: T): T {
     return obj.map((item) => resolveEnvVarsInObject(item)) as unknown as T;
   }
 
-  if (obj && typeof obj === 'object') {
+  if (typeof obj === 'object') {
     const newObj = { ...obj } as T;
     for (const key in newObj) {
       if (Object.prototype.hasOwnProperty.call(newObj, key)) {