Fix handling of empty settings (#18131)

This commit is contained in:
christine betts
2026-02-03 15:39:20 -05:00
committed by GitHub
parent 2cf3a14439
commit 3e954930f1
9 changed files with 83 additions and 13 deletions
+16 -1
View File
@@ -8,7 +8,10 @@ import { ExtensionManager } from '../../config/extension-manager.js';
import { promptForSetting } from '../../config/extensions/extensionSettings.js'; import { promptForSetting } from '../../config/extensions/extensionSettings.js';
import { loadSettings } from '../../config/settings.js'; import { loadSettings } from '../../config/settings.js';
import { requestConsentNonInteractive } from '../../config/extensions/consent.js'; import { requestConsentNonInteractive } from '../../config/extensions/consent.js';
import { debugLogger } from '@google/gemini-cli-core'; import {
debugLogger,
type ResolvedExtensionSetting,
} from '@google/gemini-cli-core';
export async function getExtensionManager() { export async function getExtensionManager() {
const workspaceDir = process.cwd(); const workspaceDir = process.cwd();
@@ -35,3 +38,15 @@ export async function getExtensionAndManager(name: string) {
return { extension, extensionManager }; return { extension, extensionManager };
} }
export function getFormattedSettingValue(
setting: ResolvedExtensionSetting,
): string {
if (!setting.value) {
return '[not set]';
}
if (setting.sensitive) {
return '***';
}
return setting.value;
}
@@ -195,7 +195,7 @@ describe('ExtensionManager Settings Scope', () => {
(s) => s.envVar === 'TEST_SETTING', (s) => s.envVar === 'TEST_SETTING',
); );
expect(setting).toBeDefined(); expect(setting).toBeDefined();
expect(setting?.value).toBe('[not set]'); expect(setting?.value).toBeUndefined();
expect(setting?.scope).toBeUndefined(); expect(setting?.scope).toBeUndefined();
// Verify output string does not contain scope // Verify output string does not contain scope
+3 -7
View File
@@ -70,6 +70,7 @@ import {
} from './extensions/extensionSettings.js'; } from './extensions/extensionSettings.js';
import type { EventEmitter } from 'node:stream'; import type { EventEmitter } from 'node:stream';
import { themeManager } from '../ui/themes/theme-manager.js'; import { themeManager } from '../ui/themes/theme-manager.js';
import { getFormattedSettingValue } from '../commands/extensions/utils.js';
interface ExtensionManagerParams { interface ExtensionManagerParams {
enabledExtensionOverrides?: string[]; enabledExtensionOverrides?: string[];
@@ -648,12 +649,7 @@ Would you like to attempt to install via "git clone" instead?`,
resolvedSettings.push({ resolvedSettings.push({
name: setting.name, name: setting.name,
envVar: setting.envVar, envVar: setting.envVar,
value: value,
value === undefined
? '[not set]'
: setting.sensitive
? '***'
: value,
sensitive: setting.sensitive ?? false, sensitive: setting.sensitive ?? false,
scope, scope,
source, source,
@@ -941,7 +937,7 @@ Would you like to attempt to install via "git clone" instead?`,
} }
scope += ')'; scope += ')';
} }
output += `\n ${setting.name}: ${setting.value} ${scope}`; output += `\n ${setting.name}: ${getFormattedSettingValue(setting)} ${scope}`;
}); });
} }
return output; return output;
@@ -786,6 +786,23 @@ describe('extensionSettings', () => {
expect(await userKeychain.getSecret('VAR2')).toBeNull(); expect(await userKeychain.getSecret('VAR2')).toBeNull();
}); });
it('should delete a non-sensitive setting if the new value is empty', async () => {
mockRequestSetting.mockResolvedValue('');
await updateSetting(
config,
'12345',
'VAR1',
mockRequestSetting,
ExtensionSettingScope.USER,
tempWorkspaceDir,
);
const expectedEnvPath = path.join(extensionDir, '.env');
const actualContent = await fsPromises.readFile(expectedEnvPath, 'utf-8');
expect(actualContent).not.toContain('VAR1=');
});
it('should not throw if deleting a non-existent sensitive setting with empty value', async () => { it('should not throw if deleting a non-existent sensitive setting with empty value', async () => {
mockRequestSetting.mockResolvedValue(''); mockRequestSetting.mockResolvedValue('');
// Ensure it doesn't exist first // Ensure it doesn't exist first
@@ -251,7 +251,11 @@ export async function updateSetting(
} }
const parsedEnv = dotenv.parse(envContent); const parsedEnv = dotenv.parse(envContent);
parsedEnv[settingToUpdate.envVar] = newValue; if (!newValue) {
delete parsedEnv[settingToUpdate.envVar];
} else {
parsedEnv[settingToUpdate.envVar] = newValue;
}
// We only want to write back the variables that are not sensitive. // We only want to write back the variables that are not sensitive.
const nonSensitiveSettings: Record<string, string> = {}; const nonSensitiveSettings: Record<string, string> = {};
@@ -9,6 +9,7 @@ import { Box, Text } from 'ink';
import { useUIState } from '../../contexts/UIStateContext.js'; import { useUIState } from '../../contexts/UIStateContext.js';
import { ExtensionUpdateState } from '../../state/extensions.js'; import { ExtensionUpdateState } from '../../state/extensions.js';
import { debugLogger, type GeminiCLIExtension } from '@google/gemini-cli-core'; import { debugLogger, type GeminiCLIExtension } from '@google/gemini-cli-core';
import { getFormattedSettingValue } from '../../../commands/extensions/utils.js';
interface ExtensionsList { interface ExtensionsList {
extensions: readonly GeminiCLIExtension[]; extensions: readonly GeminiCLIExtension[];
@@ -70,7 +71,7 @@ export const ExtensionsList: React.FC<ExtensionsList> = ({ extensions }) => {
<Text>settings:</Text> <Text>settings:</Text>
{ext.resolvedSettings.map((setting) => ( {ext.resolvedSettings.map((setting) => (
<Text key={setting.name}> <Text key={setting.name}>
- {setting.name}: {setting.value} - {setting.name}: {getFormattedSettingValue(setting)}
{setting.scope && ( {setting.scope && (
<Text color="gray"> <Text color="gray">
{' '} {' '}
+1 -1
View File
@@ -158,7 +158,7 @@ export interface ExtensionSetting {
export interface ResolvedExtensionSetting { export interface ResolvedExtensionSetting {
name: string; name: string;
envVar: string; envVar: string;
value: string; value?: string;
sensitive: boolean; sensitive: boolean;
scope?: 'user' | 'workspace'; scope?: 'user' | 'workspace';
source?: string; source?: string;
@@ -1556,6 +1556,41 @@ describe('mcp-client', () => {
expect(callArgs.env!['GEMINI_CLI_EXT_VAR']).toBe('ext-value'); expect(callArgs.env!['GEMINI_CLI_EXT_VAR']).toBe('ext-value');
}); });
it('should exclude extension settings with undefined values from environment', async () => {
const mockedTransport = vi
.spyOn(SdkClientStdioLib, 'StdioClientTransport')
.mockReturnValue({} as SdkClientStdioLib.StdioClientTransport);
await createTransport(
'test-server',
{
command: 'test-command',
extension: {
name: 'test-ext',
resolvedSettings: [
{
envVar: 'GEMINI_CLI_EXT_VAR',
value: undefined,
sensitive: false,
name: 'ext-setting',
},
],
version: '',
isActive: false,
path: '',
contextFiles: [],
id: '',
},
},
false,
EMPTY_CONFIG,
);
const callArgs = mockedTransport.mock.calls[0][0];
expect(callArgs.env).toBeDefined();
expect(callArgs.env!['GEMINI_CLI_EXT_VAR']).toBeUndefined();
});
describe('useGoogleCredentialProvider', () => { describe('useGoogleCredentialProvider', () => {
beforeEach(() => { beforeEach(() => {
// Mock GoogleAuth client // Mock GoogleAuth client
+3 -1
View File
@@ -1948,7 +1948,9 @@ function getExtensionEnvironment(
const env: Record<string, string> = {}; const env: Record<string, string> = {};
if (extension?.resolvedSettings) { if (extension?.resolvedSettings) {
for (const setting of extension.resolvedSettings) { for (const setting of extension.resolvedSettings) {
env[setting.envVar] = setting.value; if (setting.value) {
env[setting.envVar] = setting.value;
}
} }
} }
return env; return env;