diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 8f2c31bceb..141043e5c6 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -44,6 +44,7 @@ import { afterEach, type Mocked, type Mock, + fail, } from 'vitest'; import * as fs from 'node:fs'; // fs will be mocked separately import stripJsonComments from 'strip-json-comments'; // Will be mocked separately @@ -57,6 +58,7 @@ import { getSystemDefaultsPath, SETTINGS_DIRECTORY_NAME, // This is from the original module, but used by the mock. migrateSettingsToV1, + needsMigration, type Settings, loadEnvironment, } from './settings.js'; @@ -124,28 +126,7 @@ describe('Settings Loading and Merging', () => { expect(settings.system.settings).toEqual({}); expect(settings.user.settings).toEqual({}); expect(settings.workspace.settings).toEqual({}); - expect(settings.merged).toEqual({ - general: {}, - ui: { - customThemes: {}, - }, - mcp: {}, - mcpServers: {}, - context: { - includeDirectories: [], - }, - model: { - chatCompression: {}, - }, - advanced: { - excludedEnvVars: [], - }, - extensions: { - disabled: [], - workspacesWithMigrationNudge: [], - }, - security: {}, - }); + expect(settings.merged).toEqual({}); }); it('should load system settings if only system file exists', () => { @@ -179,27 +160,6 @@ describe('Settings Loading and Merging', () => { expect(settings.workspace.settings).toEqual({}); expect(settings.merged).toEqual({ ...systemSettingsContent, - general: {}, - ui: { - ...systemSettingsContent.ui, - customThemes: {}, - }, - mcp: {}, - mcpServers: {}, - context: { - includeDirectories: [], - }, - model: { - chatCompression: {}, - }, - advanced: { - excludedEnvVars: [], - }, - extensions: { - disabled: [], - workspacesWithMigrationNudge: [], - }, - security: {}, }); }); @@ -235,28 +195,6 @@ describe('Settings Loading and Merging', () => { expect(settings.workspace.settings).toEqual({}); expect(settings.merged).toEqual({ ...userSettingsContent, - general: {}, - ui: { - ...userSettingsContent.ui, - customThemes: {}, - }, - mcp: {}, - mcpServers: {}, - context: { - ...userSettingsContent.context, - includeDirectories: [], - }, - model: { - chatCompression: {}, - }, - advanced: { - excludedEnvVars: [], - }, - extensions: { - disabled: [], - workspacesWithMigrationNudge: [], - }, - security: {}, }); }); @@ -289,30 +227,7 @@ describe('Settings Loading and Merging', () => { expect(settings.user.settings).toEqual({}); expect(settings.workspace.settings).toEqual(workspaceSettingsContent); expect(settings.merged).toEqual({ - tools: { - sandbox: true, - }, - context: { - fileName: 'WORKSPACE_CONTEXT.md', - includeDirectories: [], - }, - general: {}, - ui: { - customThemes: {}, - }, - mcp: {}, - mcpServers: {}, - model: { - chatCompression: {}, - }, - advanced: { - excludedEnvVars: [], - }, - extensions: { - disabled: [], - workspacesWithMigrationNudge: [], - }, - security: {}, + ...workspaceSettingsContent, }); }); @@ -377,34 +292,20 @@ describe('Settings Loading and Merging', () => { expect(settings.user.settings).toEqual(userSettingsContent); expect(settings.workspace.settings).toEqual(workspaceSettingsContent); expect(settings.merged).toEqual({ - general: {}, ui: { theme: 'system-theme', - customThemes: {}, }, tools: { sandbox: false, + core: ['tool1'], }, telemetry: { enabled: false }, context: { fileName: 'WORKSPACE_CONTEXT.md', - includeDirectories: [], }, mcp: { allowed: ['server1', 'server2'], }, - advanced: { - excludedEnvVars: [], - }, - extensions: { - disabled: [], - workspacesWithMigrationNudge: [], - }, - mcpServers: {}, - model: { - chatCompression: {}, - }, - security: {}, }); }); @@ -446,18 +347,15 @@ describe('Settings Loading and Merging', () => { expect(settings.merged).toEqual({ ui: { theme: 'legacy-dark', - customThemes: {}, }, general: { vimMode: true, }, context: { fileName: 'LEGACY_CONTEXT.md', - includeDirectories: [], }, model: { name: 'gemini-pro', - chatCompression: {}, }, mcpServers: { 'legacy-server-1': { @@ -475,14 +373,6 @@ describe('Settings Loading and Merging', () => { allowed: ['legacy-server-1'], }, someUnrecognizedSetting: 'should-be-preserved', - advanced: { - excludedEnvVars: [], - }, - extensions: { - disabled: [], - workspacesWithMigrationNudge: [], - }, - security: {}, }); }); @@ -611,9 +501,6 @@ describe('Settings Loading and Merging', () => { expect(settings.user.settings).toEqual(userSettingsContent); expect(settings.workspace.settings).toEqual(workspaceSettingsContent); expect(settings.merged).toEqual({ - advanced: { - excludedEnvVars: [], - }, context: { fileName: 'WORKSPACE_CONTEXT.md', includeDirectories: [ @@ -624,23 +511,11 @@ describe('Settings Loading and Merging', () => { '/system/dir', ], }, - extensions: { - disabled: [], - workspacesWithMigrationNudge: [], - }, - mcp: {}, - mcpServers: {}, - model: { - chatCompression: {}, - }, - security: {}, telemetry: false, tools: { sandbox: false, }, - general: {}, ui: { - customThemes: {}, theme: 'system-theme', }, }); @@ -924,8 +799,8 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockReturnValue('{}'); const settings = loadSettings(MOCK_WORKSPACE_DIR); expect(settings.merged.telemetry).toBeUndefined(); - expect(settings.merged.ui?.customThemes).toEqual({}); - expect(settings.merged.mcpServers).toEqual({}); + expect(settings.merged.ui).toBeUndefined(); + expect(settings.merged.mcpServers).toBeUndefined(); }); it('should merge MCP servers correctly, with workspace taking precedence', () => { @@ -1050,11 +925,11 @@ describe('Settings Loading and Merging', () => { }); }); - it('should have mcpServers as empty object if not in any settings file', () => { + it('should have mcpServers as undefined if not in any settings file', () => { (mockFsExistsSync as Mock).mockReturnValue(false); // No settings files exist (fs.readFileSync as Mock).mockReturnValue('{}'); const settings = loadSettings(MOCK_WORKSPACE_DIR); - expect(settings.merged.mcpServers).toEqual({}); + expect(settings.merged.mcpServers).toBeUndefined(); }); it('should merge MCP servers from system, user, and workspace with system taking precedence', () => { @@ -1222,11 +1097,11 @@ describe('Settings Loading and Merging', () => { }); }); - it('should have chatCompression as an empty object if not in any settings file', () => { + it('should have model as undefined if not in any settings file', () => { (mockFsExistsSync as Mock).mockReturnValue(false); // No settings files exist (fs.readFileSync as Mock).mockReturnValue('{}'); const settings = loadSettings(MOCK_WORKSPACE_DIR); - expect(settings.merged.model?.chatCompression).toEqual({}); + expect(settings.merged.model).toBeUndefined(); }); it('should ignore chatCompression if contextPercentageThreshold is invalid', () => { @@ -1351,7 +1226,22 @@ describe('Settings Loading and Merging', () => { }, ); - expect(() => loadSettings(MOCK_WORKSPACE_DIR)).toThrow(FatalConfigError); + try { + loadSettings(MOCK_WORKSPACE_DIR); + fail('loadSettings should have thrown a FatalConfigError'); + } catch (e) { + expect(e).toBeInstanceOf(FatalConfigError); + const error = e as FatalConfigError; + expect(error.message).toContain( + `Error in ${USER_SETTINGS_PATH}: ${userReadError.message}`, + ); + expect(error.message).toContain( + `Error in ${MOCK_WORKSPACE_SETTINGS_PATH}: ${workspaceReadError.message}`, + ); + expect(error.message).toContain( + 'Please fix the configuration file(s) and try again.', + ); + } // Restore JSON.parse mock if it was spied on specifically for this test vi.restoreAllMocks(); // Or more targeted restore if needed @@ -1742,27 +1632,6 @@ describe('Settings Loading and Merging', () => { expect(settings.system.settings).toEqual(systemSettingsContent); expect(settings.merged).toEqual({ ...systemSettingsContent, - general: {}, - ui: { - ...systemSettingsContent.ui, - customThemes: {}, - }, - mcp: {}, - mcpServers: {}, - context: { - includeDirectories: [], - }, - model: { - chatCompression: {}, - }, - advanced: { - excludedEnvVars: [], - }, - extensions: { - disabled: [], - workspacesWithMigrationNudge: [], - }, - security: {}, }); }); }); @@ -2348,4 +2217,71 @@ describe('Settings Loading and Merging', () => { expect(process.env['TESTTEST']).not.toEqual('1234'); }); }); + + describe('needsMigration', () => { + it('should return false for an empty object', () => { + expect(needsMigration({})).toBe(false); + }); + + it('should return false for settings that are already in V2 format', () => { + const v2Settings = { + ui: { + theme: 'dark', + }, + tools: { + sandbox: true, + }, + }; + expect(needsMigration(v2Settings)).toBe(false); + }); + + it('should return true for settings with a V1 key that needs to be moved', () => { + const v1Settings = { + theme: 'dark', // v1 key + }; + expect(needsMigration(v1Settings)).toBe(true); + }); + + it('should return true for settings with a mix of V1 and V2 keys', () => { + const mixedSettings = { + theme: 'dark', // v1 key + tools: { + sandbox: true, // v2 key + }, + }; + expect(needsMigration(mixedSettings)).toBe(true); + }); + + it('should return false for settings with only V1 keys that are the same in V2', () => { + const v1Settings = { + mcpServers: {}, + telemetry: {}, + extensions: [], + }; + expect(needsMigration(v1Settings)).toBe(false); + }); + + it('should return true for settings with a mix of V1 keys that are the same in V2 and V1 keys that need moving', () => { + const v1Settings = { + mcpServers: {}, // same in v2 + theme: 'dark', // needs moving + }; + expect(needsMigration(v1Settings)).toBe(true); + }); + + it('should return false for settings with unrecognized keys', () => { + const settings = { + someUnrecognizedKey: 'value', + }; + expect(needsMigration(settings)).toBe(false); + }); + + it('should return false for settings with v2 keys and unrecognized keys', () => { + const settings = { + ui: { theme: 'dark' }, + someUnrecognizedKey: 'value', + }; + expect(needsMigration(settings)).toBe(false); + }); + }); }); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 4ad91eb4e1..2c350d7f25 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -19,9 +19,31 @@ import stripJsonComments from 'strip-json-comments'; import { DefaultLight } from '../ui/themes/default-light.js'; import { DefaultDark } from '../ui/themes/default.js'; import { isWorkspaceTrusted } from './trustedFolders.js'; -import type { Settings, MemoryImportFormat } from './settingsSchema.js'; +import { + type Settings, + type MemoryImportFormat, + SETTINGS_SCHEMA, + type MergeStrategy, + type SettingsSchema, + type SettingDefinition, +} from './settingsSchema.js'; import { resolveEnvVarsInObject } from '../utils/envVarResolver.js'; -import { mergeWith } from 'lodash-es'; +import { customDeepMerge } from '../utils/deepMerge.js'; + +function getMergeStrategyForPath(path: string[]): MergeStrategy | undefined { + let current: SettingDefinition | undefined = undefined; + let currentSchema: SettingsSchema | undefined = SETTINGS_SCHEMA; + + for (const key of path) { + if (!currentSchema || !currentSchema[key]) { + return undefined; + } + current = currentSchema[key]; + currentSchema = current.properties; + } + + return current?.mergeStrategy; +} export type { Settings, MemoryImportFormat }; @@ -35,15 +57,32 @@ const MIGRATE_V2_OVERWRITE = false; // As defined in spec.md const MIGRATION_MAP: Record = { - preferredEditor: 'general.preferredEditor', - vimMode: 'general.vimMode', + accessibility: 'ui.accessibility', + allowedTools: 'tools.allowed', + allowMCPServers: 'mcp.allowed', + autoAccept: 'tools.autoAccept', + autoConfigureMaxOldSpaceSize: 'advanced.autoConfigureMemory', + bugCommand: 'advanced.bugCommand', + chatCompression: 'model.chatCompression', + checkpointing: 'general.checkpointing', + coreTools: 'tools.core', + contextFileName: 'context.fileName', + customThemes: 'ui.customThemes', + debugKeystrokeLogging: 'general.debugKeystrokeLogging', disableAutoUpdate: 'general.disableAutoUpdate', disableUpdateNag: 'general.disableUpdateNag', - checkpointing: 'general.checkpointing', + dnsResolutionOrder: 'advanced.dnsResolutionOrder', enablePromptCompletion: 'general.enablePromptCompletion', - debugKeystrokeLogging: 'general.debugKeystrokeLogging', - theme: 'ui.theme', - customThemes: 'ui.customThemes', + enforcedAuthType: 'security.auth.enforcedType', + excludeTools: 'tools.exclude', + excludeMCPServers: 'mcp.excluded', + excludedProjectEnvVars: 'advanced.excludedEnvVars', + extensionManagement: 'advanced.extensionManagement', + extensions: 'extensions', + fileFiltering: 'context.fileFiltering', + folderTrustFeature: 'security.folderTrust.featureEnabled', + folderTrust: 'security.folderTrust.enabled', + hasSeenIdeIntegrationNudge: 'ide.hasSeenNudge', hideWindowTitle: 'ui.hideWindowTitle', hideTips: 'ui.hideTips', hideBanner: 'ui.hideBanner', @@ -55,44 +94,29 @@ const MIGRATION_MAP: Record = { showMemoryUsage: 'ui.showMemoryUsage', showLineNumbers: 'ui.showLineNumbers', showCitations: 'ui.showCitations', - accessibility: 'ui.accessibility', ideMode: 'ide.enabled', - hasSeenIdeIntegrationNudge: 'ide.hasSeenNudge', - usageStatisticsEnabled: 'privacy.usageStatisticsEnabled', - telemetry: 'telemetry', - model: 'model.name', - maxSessionTurns: 'model.maxSessionTurns', - summarizeToolOutput: 'model.summarizeToolOutput', - chatCompression: 'model.chatCompression', - skipNextSpeakerCheck: 'model.skipNextSpeakerCheck', - contextFileName: 'context.fileName', - memoryImportFormat: 'context.importFormat', - memoryDiscoveryMaxDirs: 'context.discoveryMaxDirs', includeDirectories: 'context.includeDirectories', loadMemoryFromIncludeDirectories: 'context.loadFromIncludeDirectories', - fileFiltering: 'context.fileFiltering', - useRipgrep: 'tools.useRipgrep', + maxSessionTurns: 'model.maxSessionTurns', + mcpServers: 'mcpServers', + mcpServerCommand: 'mcp.serverCommand', + memoryImportFormat: 'context.importFormat', + memoryDiscoveryMaxDirs: 'context.discoveryMaxDirs', + model: 'model.name', + preferredEditor: 'general.preferredEditor', sandbox: 'tools.sandbox', + selectedAuthType: 'security.auth.selectedType', shouldUseNodePtyShell: 'tools.usePty', - autoAccept: 'tools.autoAccept', - allowedTools: 'tools.allowed', - coreTools: 'tools.core', - excludeTools: 'tools.exclude', + skipNextSpeakerCheck: 'model.skipNextSpeakerCheck', + summarizeToolOutput: 'model.summarizeToolOutput', + telemetry: 'telemetry', + theme: 'ui.theme', toolDiscoveryCommand: 'tools.discoveryCommand', toolCallCommand: 'tools.callCommand', - mcpServerCommand: 'mcp.serverCommand', - allowMCPServers: 'mcp.allowed', - excludeMCPServers: 'mcp.excluded', - folderTrust: 'security.folderTrust.enabled', - selectedAuthType: 'security.auth.selectedType', - enforcedAuthType: 'security.auth.enforcedType', + usageStatisticsEnabled: 'privacy.usageStatisticsEnabled', useExternalAuth: 'security.auth.useExternal', - autoConfigureMaxOldSpaceSize: 'advanced.autoConfigureMemory', - dnsResolutionOrder: 'advanced.dnsResolutionOrder', - excludedProjectEnvVars: 'advanced.excludedEnvVars', - bugCommand: 'advanced.bugCommand', - extensionManagement: 'experimental.extensionManagement', - extensions: 'extensions', + useRipgrep: 'tools.useRipgrep', + vimMode: 'general.vimMode', }; export function getSystemSettingsPath(): string { @@ -175,8 +199,27 @@ function setNestedProperty( current[lastKey] = value; } -function needsMigration(settings: Record): boolean { - return !('general' in settings); +export function needsMigration(settings: Record): boolean { + // A file needs migration if it contains any top-level key that is moved to a + // nested location in V2. + const hasV1Keys = Object.entries(MIGRATION_MAP).some(([v1Key, v2Path]) => { + if (v1Key === v2Path || !(v1Key in settings)) { + return false; + } + // If a key exists that is both a V1 key and a V2 container (like 'model'), + // we need to check the type. If it's an object, it's a V2 container and not + // a V1 key that needs migration. + if ( + KNOWN_V2_CONTAINERS.has(v1Key) && + typeof settings[v1Key] === 'object' && + settings[v1Key] !== null + ) { + return false; + } + return true; + }); + + return hasV1Keys; } function migrateSettingsToV2( @@ -297,7 +340,6 @@ function mergeSettings( } : { ...restOfWorkspace, - security: {}, }; // Settings are merged with the following precedence (last one wins for @@ -306,112 +348,14 @@ function mergeSettings( // 2. User Settings // 3. Workspace Settings // 4. System Settings (as overrides) - // - // For properties that are arrays (e.g., includeDirectories), the arrays - // are concatenated. For objects (e.g., customThemes), they are merged. - return { - ...systemDefaults, - ...user, - ...safeWorkspaceWithoutFolderTrust, - ...system, - general: { - ...(systemDefaults.general || {}), - ...(user.general || {}), - ...(safeWorkspaceWithoutFolderTrust.general || {}), - ...(system.general || {}), - }, - ui: { - ...(systemDefaults.ui || {}), - ...(user.ui || {}), - ...(safeWorkspaceWithoutFolderTrust.ui || {}), - ...(system.ui || {}), - customThemes: { - ...(systemDefaults.ui?.customThemes || {}), - ...(user.ui?.customThemes || {}), - ...(safeWorkspaceWithoutFolderTrust.ui?.customThemes || {}), - ...(system.ui?.customThemes || {}), - }, - }, - security: { - ...(systemDefaults.security || {}), - ...(user.security || {}), - ...(safeWorkspaceWithoutFolderTrust.security || {}), - ...(system.security || {}), - }, - mcp: { - ...(systemDefaults.mcp || {}), - ...(user.mcp || {}), - ...(safeWorkspaceWithoutFolderTrust.mcp || {}), - ...(system.mcp || {}), - }, - mcpServers: { - ...(systemDefaults.mcpServers || {}), - ...(user.mcpServers || {}), - ...(safeWorkspaceWithoutFolderTrust.mcpServers || {}), - ...(system.mcpServers || {}), - }, - context: { - ...(systemDefaults.context || {}), - ...(user.context || {}), - ...(safeWorkspaceWithoutFolderTrust.context || {}), - ...(system.context || {}), - includeDirectories: [ - ...(systemDefaults.context?.includeDirectories || []), - ...(user.context?.includeDirectories || []), - ...(safeWorkspaceWithoutFolderTrust.context?.includeDirectories || []), - ...(system.context?.includeDirectories || []), - ], - }, - model: { - ...(systemDefaults.model || {}), - ...(user.model || {}), - ...(safeWorkspaceWithoutFolderTrust.model || {}), - ...(system.model || {}), - chatCompression: { - ...(systemDefaults.model?.chatCompression || {}), - ...(user.model?.chatCompression || {}), - ...(safeWorkspaceWithoutFolderTrust.model?.chatCompression || {}), - ...(system.model?.chatCompression || {}), - }, - }, - advanced: { - ...(systemDefaults.advanced || {}), - ...(user.advanced || {}), - ...(safeWorkspaceWithoutFolderTrust.advanced || {}), - ...(system.advanced || {}), - excludedEnvVars: [ - ...new Set([ - ...(systemDefaults.advanced?.excludedEnvVars || []), - ...(user.advanced?.excludedEnvVars || []), - ...(safeWorkspaceWithoutFolderTrust.advanced?.excludedEnvVars || []), - ...(system.advanced?.excludedEnvVars || []), - ]), - ], - }, - extensions: { - ...(systemDefaults.extensions || {}), - ...(user.extensions || {}), - ...(safeWorkspaceWithoutFolderTrust.extensions || {}), - ...(system.extensions || {}), - disabled: [ - ...new Set([ - ...(systemDefaults.extensions?.disabled || []), - ...(user.extensions?.disabled || []), - ...(safeWorkspaceWithoutFolderTrust.extensions?.disabled || []), - ...(system.extensions?.disabled || []), - ]), - ], - workspacesWithMigrationNudge: [ - ...new Set([ - ...(systemDefaults.extensions?.workspacesWithMigrationNudge || []), - ...(user.extensions?.workspacesWithMigrationNudge || []), - ...(safeWorkspaceWithoutFolderTrust.extensions - ?.workspacesWithMigrationNudge || []), - ...(system.extensions?.workspacesWithMigrationNudge || []), - ]), - ], - }, - }; + return customDeepMerge( + getMergeStrategyForPath, + {}, // Start with an empty object + systemDefaults, + user, + safeWorkspaceWithoutFolderTrust, + system, + ) as Settings; } export class LoadedSettings { @@ -687,7 +631,12 @@ export function loadSettings( } // For the initial trust check, we can only use user and system settings. - const initialTrustCheckSettings = mergeWith({}, systemSettings, userSettings); + const initialTrustCheckSettings = customDeepMerge( + getMergeStrategyForPath, + {}, + systemSettings, + userSettings, + ); const isTrusted = isWorkspaceTrusted(initialTrustCheckSettings as Settings) ?? true; diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 2bd3a354f3..7f607acc69 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -13,6 +13,17 @@ import type { } from '@google/gemini-cli-core'; import type { CustomTheme } from '../ui/themes/theme.js'; +export enum MergeStrategy { + // Replace the old value with the new value. This is the default. + REPLACE = 'replace', + // Concatenate arrays. + CONCAT = 'concat', + // Merge arrays, ensuring unique values. + UNION = 'union', + // Shallow merge objects. + SHALLOW_MERGE = 'shallow_merge', +} + export interface SettingDefinition { type: 'boolean' | 'string' | 'number' | 'array' | 'object'; label: string; @@ -25,6 +36,7 @@ export interface SettingDefinition { key?: string; properties?: SettingsSchema; showInDialog?: boolean; + mergeStrategy?: MergeStrategy; } export interface SettingsSchema { @@ -49,6 +61,7 @@ export const SETTINGS_SCHEMA = { default: {} as Record, description: 'Configuration for MCP servers.', showInDialog: false, + mergeStrategy: MergeStrategy.SHALLOW_MERGE, }, general: { @@ -485,6 +498,7 @@ export const SETTINGS_SCHEMA = { description: 'Additional directories to include in the workspace context. Missing directories will be skipped with a warning.', showInDialog: false, + mergeStrategy: MergeStrategy.CONCAT, }, loadMemoryFromIncludeDirectories: { type: 'boolean', @@ -796,6 +810,7 @@ export const SETTINGS_SCHEMA = { default: ['DEBUG', 'DEBUG_MODE'] as string[], description: 'Environment variables to exclude from project context.', showInDialog: false, + mergeStrategy: MergeStrategy.UNION, }, bugCommand: { type: 'object', @@ -847,6 +862,7 @@ export const SETTINGS_SCHEMA = { default: [] as string[], description: 'List of disabled extensions.', showInDialog: false, + mergeStrategy: MergeStrategy.UNION, }, workspacesWithMigrationNudge: { type: 'array', @@ -857,6 +873,7 @@ export const SETTINGS_SCHEMA = { description: 'List of workspaces for which the migration nudge has been shown.', showInDialog: false, + mergeStrategy: MergeStrategy.UNION, }, }, }, diff --git a/packages/cli/src/utils/deepMerge.test.ts b/packages/cli/src/utils/deepMerge.test.ts new file mode 100644 index 0000000000..0603aafb3a --- /dev/null +++ b/packages/cli/src/utils/deepMerge.test.ts @@ -0,0 +1,163 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { customDeepMerge } from './deepMerge.js'; +import { MergeStrategy } from '../config/settingsSchema.js'; + +describe('customDeepMerge', () => { + it('should merge simple objects', () => { + const target = { a: 1, b: 2 }; + const source = { b: 3, c: 4 }; + const getMergeStrategy = () => undefined; + const result = customDeepMerge(getMergeStrategy, target, source); + expect(result).toEqual({ a: 1, b: 3, c: 4 }); + }); + + it('should merge nested objects', () => { + const target = { a: { x: 1 }, b: 2 }; + const source = { a: { y: 2 }, c: 3 }; + const getMergeStrategy = () => undefined; + const result = customDeepMerge(getMergeStrategy, target, source); + expect(result).toEqual({ a: { x: 1, y: 2 }, b: 2, c: 3 }); + }); + + it('should replace arrays by default', () => { + const target = { a: [1, 2] }; + const source = { a: [3, 4] }; + const getMergeStrategy = () => undefined; + const result = customDeepMerge(getMergeStrategy, target, source); + expect(result).toEqual({ a: [3, 4] }); + }); + + it('should concatenate arrays with CONCAT strategy', () => { + const target = { a: [1, 2] }; + const source = { a: [3, 4] }; + const getMergeStrategy = (path: string[]) => + path.join('.') === 'a' ? MergeStrategy.CONCAT : undefined; + const result = customDeepMerge(getMergeStrategy, target, source); + expect(result).toEqual({ a: [1, 2, 3, 4] }); + }); + + it('should union arrays with UNION strategy', () => { + const target = { a: [1, 2, 3] }; + const source = { a: [3, 4, 5] }; + const getMergeStrategy = (path: string[]) => + path.join('.') === 'a' ? MergeStrategy.UNION : undefined; + const result = customDeepMerge(getMergeStrategy, target, source); + expect(result).toEqual({ a: [1, 2, 3, 4, 5] }); + }); + + it('should shallow merge objects with SHALLOW_MERGE strategy', () => { + const target = { a: { x: 1, y: 1 } }; + const source = { a: { y: 2, z: 2 } }; + const getMergeStrategy = (path: string[]) => + path.join('.') === 'a' ? MergeStrategy.SHALLOW_MERGE : undefined; + const result = customDeepMerge(getMergeStrategy, target, source); + // This is still a deep merge, but the properties of the object are merged. + expect(result).toEqual({ a: { x: 1, y: 2, z: 2 } }); + }); + + it('should handle multiple source objects', () => { + const target = { a: 1 }; + const source1 = { b: 2 }; + const source2 = { c: 3 }; + const getMergeStrategy = () => undefined; + const result = customDeepMerge(getMergeStrategy, target, source1, source2); + expect(result).toEqual({ a: 1, b: 2, c: 3 }); + }); + + it('should return an empty object if no sources are provided', () => { + const getMergeStrategy = () => undefined; + const result = customDeepMerge(getMergeStrategy); + expect(result).toEqual({}); + }); + + it('should return a deep copy of the first source if only one is provided', () => { + const target = { a: { b: 1 } }; + const getMergeStrategy = () => undefined; + const result = customDeepMerge(getMergeStrategy, target); + expect(result).toEqual(target); + expect(result).not.toBe(target); + }); + + it('should not mutate the original source objects', () => { + const target = { a: { x: 1 }, b: [1, 2] }; + const source = { a: { y: 2 }, b: [3, 4] }; + const originalTarget = JSON.parse(JSON.stringify(target)); + const originalSource = JSON.parse(JSON.stringify(source)); + const getMergeStrategy = () => undefined; + + customDeepMerge(getMergeStrategy, target, source); + + expect(target).toEqual(originalTarget); + expect(source).toEqual(originalSource); + }); + + it('should not mutate sources when merging multiple levels deep', () => { + const s1 = { data: { common: { val: 'from s1' }, s1_only: true } }; + const s2 = { data: { common: { val: 'from s2' }, s2_only: true } }; + const s1_original = JSON.parse(JSON.stringify(s1)); + const s2_original = JSON.parse(JSON.stringify(s2)); + + const getMergeStrategy = () => undefined; + const result = customDeepMerge(getMergeStrategy, s1, s2); + + expect(s1).toEqual(s1_original); + expect(s2).toEqual(s2_original); + expect(result).toEqual({ + data: { + common: { val: 'from s2' }, + s1_only: true, + s2_only: true, + }, + }); + }); + + it('should handle complex nested strategies', () => { + const target = { + level1: { + arr1: [1, 2], + arr2: [1, 2], + obj1: { a: 1 }, + }, + }; + const source = { + level1: { + arr1: [3, 4], + arr2: [2, 3], + obj1: { b: 2 }, + }, + }; + const getMergeStrategy = (path: string[]) => { + const p = path.join('.'); + if (p === 'level1.arr1') return MergeStrategy.CONCAT; + if (p === 'level1.arr2') return MergeStrategy.UNION; + if (p === 'level1.obj1') return MergeStrategy.SHALLOW_MERGE; + return undefined; + }; + + const result = customDeepMerge(getMergeStrategy, target, source); + + expect(result).toEqual({ + level1: { + arr1: [1, 2, 3, 4], + arr2: [1, 2, 3], + obj1: { a: 1, b: 2 }, + }, + }); + }); + + it('should not pollute the prototype', () => { + const maliciousSource = JSON.parse('{"__proto__": {"polluted": "true"}}'); + const getMergeStrategy = () => undefined; + const result = customDeepMerge(getMergeStrategy, {}, maliciousSource); + + expect(result).toEqual({}); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect(({} as any).polluted).toBeUndefined(); + }); +}); diff --git a/packages/cli/src/utils/deepMerge.ts b/packages/cli/src/utils/deepMerge.ts new file mode 100644 index 0000000000..e83c7a5203 --- /dev/null +++ b/packages/cli/src/utils/deepMerge.ts @@ -0,0 +1,90 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { MergeStrategy } from '../config/settingsSchema.js'; + +type Mergeable = + | string + | number + | boolean + | null + | undefined + | object + | Mergeable[]; + +type MergeableObject = Record; + +function isPlainObject(item: unknown): item is MergeableObject { + return !!item && typeof item === 'object' && !Array.isArray(item); +} + +function mergeRecursively( + target: MergeableObject, + source: MergeableObject, + getMergeStrategyForPath: (path: string[]) => MergeStrategy | undefined, + path: string[] = [], +) { + for (const key of Object.keys(source)) { + if (key === '__proto__' || key === 'constructor' || key === 'prototype') { + continue; + } + const newPath = [...path, key]; + const srcValue = source[key]; + const objValue = target[key]; + const mergeStrategy = getMergeStrategyForPath(newPath); + + if (mergeStrategy === MergeStrategy.SHALLOW_MERGE && objValue && srcValue) { + const obj1 = + typeof objValue === 'object' && objValue !== null ? objValue : {}; + const obj2 = + typeof srcValue === 'object' && srcValue !== null ? srcValue : {}; + target[key] = { ...obj1, ...obj2 }; + continue; + } + + if (Array.isArray(objValue)) { + const srcArray = Array.isArray(srcValue) ? srcValue : [srcValue]; + if (mergeStrategy === MergeStrategy.CONCAT) { + target[key] = objValue.concat(srcArray); + continue; + } + if (mergeStrategy === MergeStrategy.UNION) { + target[key] = [...new Set(objValue.concat(srcArray))]; + continue; + } + } + + if (isPlainObject(objValue) && isPlainObject(srcValue)) { + mergeRecursively(objValue, srcValue, getMergeStrategyForPath, newPath); + } else if (isPlainObject(srcValue)) { + target[key] = {}; + mergeRecursively( + target[key] as MergeableObject, + srcValue, + getMergeStrategyForPath, + newPath, + ); + } else { + target[key] = srcValue; + } + } + return target; +} + +export function customDeepMerge( + getMergeStrategyForPath: (path: string[]) => MergeStrategy | undefined, + ...sources: MergeableObject[] +): MergeableObject { + const result: MergeableObject = {}; + + for (const source of sources) { + if (source) { + mergeRecursively(result, source, getMergeStrategyForPath); + } + } + + return result; +} diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index aa86599976..8f91d5fc48 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import type { Mock } from 'vitest'; import type { ConfigParameters, SandboxConfig } from './config.js'; import { Config, ApprovalMode } from './config.js'; @@ -671,6 +671,90 @@ describe('Server Config (config.ts)', () => { ).mock.calls.some((call) => call[0] instanceof vi.mocked(ReadFileTool)); expect(wasReadFileToolRegistered).toBe(false); }); + + describe('with minified tool class names', () => { + beforeEach(() => { + Object.defineProperty( + vi.mocked(ShellTool).prototype.constructor, + 'name', + { + value: '_ShellTool', + configurable: true, + }, + ); + }); + + afterEach(() => { + Object.defineProperty( + vi.mocked(ShellTool).prototype.constructor, + 'name', + { + value: 'ShellTool', + }, + ); + }); + + it('should register a tool if coreTools contains the non-minified class name', async () => { + const params: ConfigParameters = { + ...baseParams, + coreTools: ['ShellTool'], + }; + const config = new Config(params); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerTool: Mock } }; + } + ).ToolRegistry.prototype.registerTool; + + const wasShellToolRegistered = ( + registerToolMock as Mock + ).mock.calls.some((call) => call[0] instanceof vi.mocked(ShellTool)); + expect(wasShellToolRegistered).toBe(true); + }); + + it('should not register a tool if excludeTools contains the non-minified class name', async () => { + const params: ConfigParameters = { + ...baseParams, + coreTools: undefined, // all tools enabled by default + excludeTools: ['ShellTool'], + }; + const config = new Config(params); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerTool: Mock } }; + } + ).ToolRegistry.prototype.registerTool; + + const wasShellToolRegistered = ( + registerToolMock as Mock + ).mock.calls.some((call) => call[0] instanceof vi.mocked(ShellTool)); + expect(wasShellToolRegistered).toBe(false); + }); + + it('should register a tool if coreTools contains an argument-specific pattern with the non-minified class name', async () => { + const params: ConfigParameters = { + ...baseParams, + coreTools: ['ShellTool(git status)'], + }; + const config = new Config(params); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerTool: Mock } }; + } + ).ToolRegistry.prototype.registerTool; + + const wasShellToolRegistered = ( + registerToolMock as Mock + ).mock.calls.some((call) => call[0] instanceof vi.mocked(ShellTool)); + expect(wasShellToolRegistered).toBe(true); + }); + }); }); }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 2cb2d3eee9..543199274f 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -844,20 +844,22 @@ export class Config { const toolName = ToolClass.Name || className; const coreTools = this.getCoreTools(); const excludeTools = this.getExcludeTools() || []; + // On some platforms, the className can be minified to _ClassName. + const normalizedClassName = className.replace(/^_+/, ''); let isEnabled = true; // Enabled by default if coreTools is not set. if (coreTools) { isEnabled = coreTools.some( (tool) => - tool === className || tool === toolName || - tool.startsWith(`${className}(`) || - tool.startsWith(`${toolName}(`), + tool === normalizedClassName || + tool.startsWith(`${toolName}(`) || + tool.startsWith(`${normalizedClassName}(`), ); } const isExcluded = excludeTools.some( - (tool) => tool === className || tool === toolName, + (tool) => tool === toolName || tool === normalizedClassName, ); if (isExcluded) {