From 4c67eef0f2993c946f7f7afa0ac9149be25fbf3a Mon Sep 17 00:00:00 2001 From: christine betts Date: Tue, 6 Jan 2026 12:26:01 -0500 Subject: [PATCH] Inform user of missing settings on extensions update (#15944) Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- packages/cli/src/config/extension-manager.ts | 35 +- packages/cli/src/config/extension.test.ts | 21 +- .../config/extensions/extensionSettings.ts | 21 ++ .../extensions/extensionUpdates.test.ts | 308 ++++++++++++++++++ 4 files changed, 357 insertions(+), 28 deletions(-) create mode 100644 packages/cli/src/config/extensions/extensionUpdates.test.ts diff --git a/packages/cli/src/config/extension-manager.ts b/packages/cli/src/config/extension-manager.ts index 3dcd71dac4..8c1c8e0a77 100644 --- a/packages/cli/src/config/extension-manager.ts +++ b/packages/cli/src/config/extension-manager.ts @@ -46,6 +46,7 @@ import { type HookDefinition, type HookEventName, type ResolvedExtensionSetting, + coreEvents, } from '@google/gemini-cli-core'; import { maybeRequestConsentOrFail } from './extensions/consent.js'; import { resolveEnvVarsInObject } from '../utils/envVarResolver.js'; @@ -59,6 +60,7 @@ import { import { getEnvContents, maybePromptForSettings, + getMissingSettings, type ExtensionSetting, } from './extensions/extensionSettings.js'; import type { EventEmitter } from 'node:stream'; @@ -227,25 +229,6 @@ Would you like to attempt to install via "git clone" instead?`, try { newExtensionConfig = await this.loadExtensionConfig(localSourcePath); - if (isUpdate && installMetadata.autoUpdate) { - const oldSettings = new Set( - previousExtensionConfig.settings?.map((s) => s.name) || [], - ); - const newSettings = new Set( - newExtensionConfig.settings?.map((s) => s.name) || [], - ); - - const settingsAreEqual = - oldSettings.size === newSettings.size && - [...oldSettings].every((value) => newSettings.has(value)); - - if (!settingsAreEqual && installMetadata.autoUpdate) { - throw new Error( - `Extension "${newExtensionConfig.name}" has settings changes and cannot be auto-updated. Please update manually.`, - ); - } - } - const newExtensionName = newExtensionConfig.name; const previous = this.getExtensions().find( (installed) => installed.name === newExtensionName, @@ -316,6 +299,20 @@ Would you like to attempt to install via "git clone" instead?`, } } + const missingSettings = await getMissingSettings( + newExtensionConfig, + extensionId, + ); + if (missingSettings.length > 0) { + const message = `Extension "${newExtensionConfig.name}" has missing settings: ${missingSettings + .map((s) => s.name) + .join( + ', ', + )}. Please run "gemini extensions settings ${newExtensionConfig.name} " to configure them.`; + debugLogger.warn(message); + coreEvents.emitFeedback('warning', message); + } + if ( installMetadata.type === 'local' || installMetadata.type === 'git' || diff --git a/packages/cli/src/config/extension.test.ts b/packages/cli/src/config/extension.test.ts index b8aa336b5d..530d171cd7 100644 --- a/packages/cli/src/config/extension.test.ts +++ b/packages/cli/src/config/extension.test.ts @@ -1447,7 +1447,7 @@ This extension will run the following MCP servers: expect(envContent).toContain('NEW_SETTING=new-setting-value'); }); - it('should fail auto-update if settings have changed', async () => { + it('should auto-update if settings have changed', async () => { // 1. Install initial version with autoUpdate: true const oldSourceExtDir = createExtension({ extensionsDir: tempHomeDir, @@ -1469,7 +1469,7 @@ This extension will run the following MCP servers: }); // 2. Create new version with different settings - const newSourceExtDir = createExtension({ + const extensionDir = createExtension({ extensionsDir: tempHomeDir, name: 'my-auto-update-ext', version: '1.1.0', @@ -1488,14 +1488,17 @@ This extension will run the following MCP servers: ); // 3. Attempt to update and assert it fails - await expect( - extensionManager.installOrUpdateExtension( - { source: newSourceExtDir, type: 'local', autoUpdate: true }, - previousExtensionConfig, - ), - ).rejects.toThrow( - 'Extension "my-auto-update-ext" has settings changes and cannot be auto-updated. Please update manually.', + const updatedExtension = await extensionManager.installOrUpdateExtension( + { + source: extensionDir, + type: 'local', + autoUpdate: true, + }, + previousExtensionConfig, ); + + expect(updatedExtension.version).toBe('1.1.0'); + expect(extensionManager.getExtensions()[0].version).toBe('1.1.0'); }); it('should throw an error for invalid extension names', async () => { diff --git a/packages/cli/src/config/extensions/extensionSettings.ts b/packages/cli/src/config/extensions/extensionSettings.ts index eb953d19c5..582a94c807 100644 --- a/packages/cli/src/config/extensions/extensionSettings.ts +++ b/packages/cli/src/config/extensions/extensionSettings.ts @@ -298,3 +298,24 @@ async function clearSettings( } return; } + +export async function getMissingSettings( + extensionConfig: ExtensionConfig, + extensionId: string, +): Promise { + const { settings } = extensionConfig; + if (!settings || settings.length === 0) { + return []; + } + + const existingSettings = await getEnvContents(extensionConfig, extensionId); + const missingSettings: ExtensionSetting[] = []; + + for (const setting of settings) { + if (existingSettings[setting.envVar] === undefined) { + missingSettings.push(setting); + } + } + + return missingSettings; +} diff --git a/packages/cli/src/config/extensions/extensionUpdates.test.ts b/packages/cli/src/config/extensions/extensionUpdates.test.ts new file mode 100644 index 0000000000..3ba11981bb --- /dev/null +++ b/packages/cli/src/config/extensions/extensionUpdates.test.ts @@ -0,0 +1,308 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as path from 'node:path'; +import * as os from 'node:os'; +import * as fs from 'node:fs'; +import { getMissingSettings } from './extensionSettings.js'; +import type { ExtensionConfig } from '../extension.js'; +import { ExtensionStorage } from './storage.js'; +import { + KeychainTokenStorage, + debugLogger, + type ExtensionInstallMetadata, + type GeminiCLIExtension, + coreEvents, +} from '@google/gemini-cli-core'; +import { EXTENSION_SETTINGS_FILENAME } from './variables.js'; +import { ExtensionManager } from '../extension-manager.js'; + +vi.mock('node:fs', async (importOriginal) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const actual = await importOriginal(); + return { + ...actual, + default: { + ...actual.default, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + existsSync: vi.fn((...args: any[]) => actual.existsSync(...args)), + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + existsSync: vi.fn((...args: any[]) => actual.existsSync(...args)), + }; +}); + +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + KeychainTokenStorage: vi.fn(), + debugLogger: { + warn: vi.fn(), + error: vi.fn(), + log: vi.fn(), + }, + coreEvents: { + emitFeedback: vi.fn(), // Mock emitFeedback + on: vi.fn(), + off: vi.fn(), + }, + }; +}); + +// Mock os.homedir because ExtensionStorage uses it +vi.mock('os', async (importOriginal) => { + const mockedOs = await importOriginal(); + return { + ...mockedOs, + homedir: vi.fn(), + }; +}); + +describe('extensionUpdates', () => { + let tempHomeDir: string; + let tempWorkspaceDir: string; + let extensionDir: string; + let mockKeychainData: Record>; + + beforeEach(() => { + vi.clearAllMocks(); + mockKeychainData = {}; + + // Mock Keychain + vi.mocked(KeychainTokenStorage).mockImplementation( + (serviceName: string) => { + if (!mockKeychainData[serviceName]) { + mockKeychainData[serviceName] = {}; + } + const keychainData = mockKeychainData[serviceName]; + return { + getSecret: vi + .fn() + .mockImplementation( + async (key: string) => keychainData[key] || null, + ), + setSecret: vi + .fn() + .mockImplementation(async (key: string, value: string) => { + keychainData[key] = value; + }), + deleteSecret: vi.fn().mockImplementation(async (key: string) => { + delete keychainData[key]; + }), + listSecrets: vi + .fn() + .mockImplementation(async () => Object.keys(keychainData)), + isAvailable: vi.fn().mockResolvedValue(true), + } as unknown as KeychainTokenStorage; + }, + ); + + // Setup Temp Dirs + tempHomeDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'gemini-cli-test-home-'), + ); + tempWorkspaceDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'gemini-cli-test-workspace-'), + ); + extensionDir = path.join(tempHomeDir, '.gemini', 'extensions', 'test-ext'); + + // Mock ExtensionStorage to rely on our temp extension dir + vi.spyOn(ExtensionStorage.prototype, 'getExtensionDir').mockReturnValue( + extensionDir, + ); + // Mock getEnvFilePath is checking extensionDir/variables.env? No, it used ExtensionStorage logic. + // getEnvFilePath in extensionSettings.ts: + // if workspace, process.cwd()/.env (we need to mock process.cwd or move tempWorkspaceDir there) + // if user, ExtensionStorage(name).getEnvFilePath() -> joins extensionDir + '.env' + + fs.mkdirSync(extensionDir, { recursive: true }); + vi.mocked(os.homedir).mockReturnValue(tempHomeDir); + vi.spyOn(process, 'cwd').mockReturnValue(tempWorkspaceDir); + }); + + afterEach(() => { + fs.rmSync(tempHomeDir, { recursive: true, force: true }); + fs.rmSync(tempWorkspaceDir, { recursive: true, force: true }); + vi.restoreAllMocks(); + }); + + describe('getMissingSettings', () => { + it('should return empty list if all settings are present', async () => { + const config: ExtensionConfig = { + name: 'test-ext', + version: '1.0.0', + settings: [ + { name: 's1', description: 'd1', envVar: 'VAR1' }, + { name: 's2', description: 'd2', envVar: 'VAR2', sensitive: true }, + ], + }; + const extensionId = '12345'; + + // Setup User Env + const userEnvPath = path.join(extensionDir, EXTENSION_SETTINGS_FILENAME); + fs.writeFileSync(userEnvPath, 'VAR1=val1'); + + // Setup Keychain + const userKeychain = new KeychainTokenStorage( + `Gemini CLI Extensions test-ext ${extensionId}`, + ); + await userKeychain.setSecret('VAR2', 'val2'); + + const missing = await getMissingSettings(config, extensionId); + expect(missing).toEqual([]); + }); + + it('should identify missing non-sensitive settings', async () => { + const config: ExtensionConfig = { + name: 'test-ext', + version: '1.0.0', + settings: [{ name: 's1', description: 'd1', envVar: 'VAR1' }], + }; + const extensionId = '12345'; + + const missing = await getMissingSettings(config, extensionId); + expect(missing).toHaveLength(1); + expect(missing[0].name).toBe('s1'); + }); + + it('should identify missing sensitive settings', async () => { + const config: ExtensionConfig = { + name: 'test-ext', + version: '1.0.0', + settings: [ + { name: 's2', description: 'd2', envVar: 'VAR2', sensitive: true }, + ], + }; + const extensionId = '12345'; + + const missing = await getMissingSettings(config, extensionId); + expect(missing).toHaveLength(1); + expect(missing[0].name).toBe('s2'); + }); + + it('should respect settings present in workspace', async () => { + const config: ExtensionConfig = { + name: 'test-ext', + version: '1.0.0', + settings: [{ name: 's1', description: 'd1', envVar: 'VAR1' }], + }; + const extensionId = '12345'; + + // Setup Workspace Env + const workspaceEnvPath = path.join( + tempWorkspaceDir, + EXTENSION_SETTINGS_FILENAME, + ); + fs.writeFileSync(workspaceEnvPath, 'VAR1=val1'); + + const missing = await getMissingSettings(config, extensionId); + expect(missing).toEqual([]); + }); + }); + + describe('ExtensionManager integration', () => { + it('should warn about missing settings after update', async () => { + // Mock ExtensionManager methods to avoid FS/Network usage + const newConfig: ExtensionConfig = { + name: 'test-ext', + version: '1.1.0', + settings: [{ name: 's1', description: 'd1', envVar: 'VAR1' }], + }; + + const previousConfig: ExtensionConfig = { + name: 'test-ext', + version: '1.0.0', + settings: [], + }; + + const installMetadata: ExtensionInstallMetadata = { + source: extensionDir, + type: 'local', + autoUpdate: true, + }; + + const manager = new ExtensionManager({ + workspaceDir: tempWorkspaceDir, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + settings: { telemetry: {} } as any, + requestConsent: vi.fn().mockResolvedValue(true), + requestSetting: null, // Simulate non-interactive + }); + + // Mock methods called by installOrUpdateExtension + vi.spyOn(manager, 'loadExtensionConfig').mockResolvedValue(newConfig); + vi.spyOn(manager, 'getExtensions').mockReturnValue([ + { + name: 'test-ext', + version: '1.0.0', + installMetadata, + path: extensionDir, + // Mocks for other required props + contextFiles: [], + mcpServers: {}, + hooks: undefined, + isActive: true, + id: 'test-id', + settings: [], + resolvedSettings: [], + skills: [], + } as unknown as GeminiCLIExtension, + ]); + vi.spyOn(manager, 'uninstallExtension').mockResolvedValue(undefined); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.spyOn(manager as any, 'loadExtension').mockResolvedValue( + {} as unknown as GeminiCLIExtension, + ); + vi.spyOn(manager, 'enableExtension').mockResolvedValue(undefined); + + // Mock fs.promises for the operations inside installOrUpdateExtension + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + vi.spyOn(fs.promises, 'writeFile').mockResolvedValue(undefined); + vi.spyOn(fs.promises, 'rm').mockResolvedValue(undefined); + vi.mocked(fs.existsSync).mockReturnValue(false); // No hooks + + // Mock copyExtension? It's imported. + // We can rely on ignoring the failure if we mock enough. + // Actually copyExtension is called. We need to mock it if it does real IO. + // But we can just let it fail or mock fs.cp if it uses it. + // Let's assume the other mocks cover the critical path to the warning. + // Warning happens BEFORE copyExtension? + // No, warning is after copyExtension usually. + // But in my code: + // const missingSettings = await getMissingSettings(...) + // if (missingSettings.length > 0) debugLogger.warn(...) + // ... + // copyExtension(...) + + // Wait, let's check extension-manager.ts order. + // Line 303: getMissingSettings + // Line 317: if (installMetadata.type === 'local' ...) copyExtension + + // So getMissingSettings is called BEFORE copyExtension. + + try { + await manager.installOrUpdateExtension(installMetadata, previousConfig); + } catch (_) { + // Ignore errors from copyExtension or others, we just want to verify the warning + } + + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining( + 'Extension "test-ext" has missing settings: s1', + ), + ); + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'warning', + expect.stringContaining( + 'Please run "gemini extensions settings test-ext "', + ), + ); + }); + }); +});