mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-05-17 23:32:43 -07:00
refactor(config): stabilize synchronous experiment access and add real-time updates
- Revert getExperimentValue to a synchronous interface to maintain compatibility with existing callers. - Add Config.updateExperimentalSettings() to support immediate application of /experiment set/unset changes. - Implement CLI argument normalization to handle kebab-case, camelCase, and snake_case consistently. - Enhance getExperimentFlagIdFromName to be more resilient to different naming conventions. - Rename getNumericalRoutingEnabled to isNumericalRoutingEnabled for better idiomatic consistency. - Update documentation in experimentation skill to recommend strongly-typed wrappers in Config.ts. - Add regression tests for CLI overrides and update all relevant routing and command tests. - Fix lint errors by removing unnecessary await/Promise.all for synchronous config methods.
This commit is contained in:
@@ -23,8 +23,17 @@ When a user asks to add a new experiment, follow these steps sequentially:
|
||||
- **Note:** The key in `ExperimentFlags` (converted to kebab-case) will be the name used for CLI flags and Settings. For example, `MY_NEW_FEATURE` becomes `my-new-feature`.
|
||||
|
||||
### 2. Usage in Code
|
||||
- **Method:** `config.getExperimentValue<Type>(ExperimentFlags.YOUR_FLAG_ID)`
|
||||
- This method is dynamic. You do **not** need to update the `Config` class or Yargs for every new flag.
|
||||
- **Generic Method:** `config.getExperimentValue<Type>(ExperimentFlags.YOUR_FLAG_ID)`
|
||||
- **Preferred Pattern (Strongly Typed Wrappers):** To maintain a clean and discoverable interface, you should add a strongly typed wrapper method in `packages/core/src/config/config.ts`. This allows other developers to easily find and use your experiment with proper type safety.
|
||||
|
||||
Example in `Config` class:
|
||||
```typescript
|
||||
isNewFeatureEnabled(): boolean {
|
||||
return this.getExperimentValue<boolean>(ExperimentFlags.MY_NEW_FEATURE) ?? false;
|
||||
}
|
||||
```
|
||||
|
||||
- **Dynamic Nature:** While `getExperimentValue` is dynamic and doesn't *require* a `Config` update for every flag, the wrapper pattern is highly encouraged for long-term maintainability.
|
||||
- **CLI Override:** Users can override via `--experiment your-flag-name=value`.
|
||||
- **Settings Override:** Users can override in their `settings.json`:
|
||||
```json
|
||||
|
||||
@@ -889,36 +889,46 @@ export async function loadCliConfig(
|
||||
const [key, ...valueParts] = entry.split('=');
|
||||
const value = valueParts.join('=');
|
||||
if (key && value !== undefined) {
|
||||
// Normalize key to kebab-case (e.g., enableNumericalRouting -> enable-numerical-routing)
|
||||
const normalizedKey = key
|
||||
.replace(/([a-z])([A-Z])/g, '$1-$2')
|
||||
.toLowerCase()
|
||||
.replace(/_/g, '-');
|
||||
|
||||
// Simple type inference for CLI args
|
||||
if (value === 'true') experimentalCliArgs[key] = true;
|
||||
else if (value === 'false') experimentalCliArgs[key] = false;
|
||||
if (value === 'true') experimentalCliArgs[normalizedKey] = true;
|
||||
else if (value === 'false') experimentalCliArgs[normalizedKey] = false;
|
||||
else if (!isNaN(Number(value)))
|
||||
experimentalCliArgs[key] = Number(value);
|
||||
else experimentalCliArgs[key] = value;
|
||||
experimentalCliArgs[normalizedKey] = Number(value);
|
||||
else experimentalCliArgs[normalizedKey] = value;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
let clientName: string | undefined = undefined;
|
||||
if (isAcpMode) {
|
||||
const ide = detectIdeFromEnv();
|
||||
if (
|
||||
ide &&
|
||||
(ide.name !== 'vscode' || process.env['TERM_PROGRAM'] === 'vscode')
|
||||
) {
|
||||
clientName = `acp-${ide.name}`;
|
||||
}
|
||||
let clientName: string | undefined = undefined;
|
||||
if (isAcpMode) {
|
||||
const ide = detectIdeFromEnv();
|
||||
if (
|
||||
ide &&
|
||||
(ide.name !== 'vscode' || process.env['TERM_PROGRAM'] === 'vscode')
|
||||
) {
|
||||
clientName = `acp-${ide.name}`;
|
||||
}
|
||||
}
|
||||
|
||||
const useGeneralistProfile =
|
||||
settings.experimental?.generalistProfile ?? false;
|
||||
const useContextManagement =
|
||||
settings.experimental?.contextManagement ?? false;
|
||||
const contextManagement = {
|
||||
...(useGeneralistProfile ? generalistProfile : {}),
|
||||
...(useContextManagement ? settings?.contextManagement : {}),
|
||||
enabled: useContextManagement || useGeneralistProfile,
|
||||
};
|
||||
const useGeneralistProfile =
|
||||
settings.experimental?.generalistProfile ?? false;
|
||||
const useContextManagement =
|
||||
settings.experimental?.contextManagement ?? false;
|
||||
const contextManagement = {
|
||||
...(useGeneralistProfile ? generalistProfile : {}),
|
||||
...(useContextManagement ? settings?.contextManagement : {}),
|
||||
enabled: useContextManagement || useGeneralistProfile,
|
||||
};
|
||||
|
||||
if (debugMode && Object.keys(experimentalCliArgs).length > 0) {
|
||||
debugLogger.debug('Experimental CLI args:', experimentalCliArgs);
|
||||
}
|
||||
return new Config({
|
||||
acpMode: isAcpMode,
|
||||
clientName,
|
||||
|
||||
@@ -120,7 +120,7 @@ export const createMockConfig = (overrides: Partial<Config> = {}): Config =>
|
||||
isTrustedFolder: vi.fn().mockReturnValue(true),
|
||||
getCompressionThreshold: vi.fn().mockResolvedValue(undefined),
|
||||
getUserCaching: vi.fn().mockResolvedValue(false),
|
||||
getNumericalRoutingEnabled: vi.fn().mockResolvedValue(false),
|
||||
isNumericalRoutingEnabled: vi.fn().mockReturnValue(false),
|
||||
getClassifierThreshold: vi.fn().mockResolvedValue(undefined),
|
||||
getBannerTextNoCapacityIssues: vi.fn().mockResolvedValue(''),
|
||||
getBannerTextCapacityIssues: vi.fn().mockResolvedValue(''),
|
||||
|
||||
@@ -18,6 +18,7 @@ describe('experimentCommand', () => {
|
||||
services: {
|
||||
config: {
|
||||
getExperimentValue: vi.fn(),
|
||||
updateExperimentalSettings: vi.fn(),
|
||||
},
|
||||
settings: {
|
||||
merged: {
|
||||
|
||||
@@ -107,13 +107,16 @@ const setExperimentCommand: SlashCommand = {
|
||||
value = rawValue;
|
||||
}
|
||||
|
||||
const { settings } = context.services;
|
||||
const { settings, config } = context.services;
|
||||
if (!config) return;
|
||||
|
||||
const currentExperimental = {
|
||||
...((settings.merged.experimental as Record<string, unknown>) || {}),
|
||||
};
|
||||
currentExperimental[name] = value;
|
||||
|
||||
settings.setValue(SettingScope.User, 'experimental', currentExperimental);
|
||||
config.updateExperimentalSettings(currentExperimental);
|
||||
|
||||
context.ui.addItem({
|
||||
type: MessageType.INFO,
|
||||
@@ -138,7 +141,9 @@ const unsetExperimentCommand: SlashCommand = {
|
||||
return;
|
||||
}
|
||||
|
||||
const { settings } = context.services;
|
||||
const { settings, config } = context.services;
|
||||
if (!config) return;
|
||||
|
||||
const currentExperimental = {
|
||||
...((settings.merged.experimental as Record<string, unknown>) || {}),
|
||||
};
|
||||
@@ -153,6 +158,7 @@ const unsetExperimentCommand: SlashCommand = {
|
||||
|
||||
delete currentExperimental[name];
|
||||
settings.setValue(SettingScope.User, 'experimental', currentExperimental);
|
||||
config.updateExperimentalSettings(currentExperimental);
|
||||
|
||||
context.ui.addItem({
|
||||
type: MessageType.INFO,
|
||||
|
||||
@@ -102,9 +102,13 @@ export function getExperimentFlagName(flagId: number): string | undefined {
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets the ID of an experiment flag from its kebab-case name.
|
||||
* Gets the ID of an experiment flag from its name (supports kebab-case or camelCase).
|
||||
*/
|
||||
export function getExperimentFlagIdFromName(name: string): number | undefined {
|
||||
const constantName = name.toUpperCase().replace(/-/g, '_');
|
||||
// Convert enableNumericalRouting or enable-numerical-routing to ENABLE_NUMERICAL_ROUTING
|
||||
const constantName = name
|
||||
.replace(/([a-z])([A-Z])/g, '$1_$2') // camelCase to snake_case
|
||||
.toUpperCase()
|
||||
.replace(/-/g, '_'); // kebab-case to snake_case
|
||||
return (ExperimentFlags as Record<string, number>)[constantName];
|
||||
}
|
||||
|
||||
@@ -0,0 +1,49 @@
|
||||
/**
|
||||
* @license
|
||||
* Copyright 2026 Google LLC
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { Config } from './config.js';
|
||||
import { ExperimentFlags } from '../code_assist/experiments/flagNames.js';
|
||||
|
||||
describe('Config CLI Override', () => {
|
||||
const sessionId = 'test-session';
|
||||
const targetDir = process.cwd();
|
||||
const cwd = process.cwd();
|
||||
const model = 'gemini-pro';
|
||||
|
||||
it('should prioritize CLI argument over local setting', () => {
|
||||
const config = new Config({
|
||||
sessionId,
|
||||
targetDir,
|
||||
cwd,
|
||||
model,
|
||||
debugMode: false,
|
||||
experimentalCliArgs: { 'enable-numerical-routing': true },
|
||||
experimentalSettings: { 'enable-numerical-routing': false },
|
||||
});
|
||||
|
||||
expect(config.isNumericalRoutingEnabled()).toBe(true);
|
||||
});
|
||||
|
||||
it('should prioritize CLI argument over remote experiment', () => {
|
||||
const config = new Config({
|
||||
sessionId,
|
||||
targetDir,
|
||||
cwd,
|
||||
model,
|
||||
debugMode: false,
|
||||
experimentalCliArgs: { 'enable-numerical-routing': true },
|
||||
experiments: {
|
||||
flags: {
|
||||
[ExperimentFlags.ENABLE_NUMERICAL_ROUTING]: { boolValue: false },
|
||||
},
|
||||
experimentIds: [],
|
||||
},
|
||||
});
|
||||
|
||||
expect(config.isNumericalRoutingEnabled()).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -942,7 +942,7 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
|
||||
private readonly enableAgents: boolean;
|
||||
private agents: AgentSettings;
|
||||
private readonly experimentalSettings: Record<string, unknown>;
|
||||
private experimentalSettings: Record<string, unknown>;
|
||||
private readonly experimentalCliArgs: Record<string, unknown>;
|
||||
private readonly enableEventDrivenScheduler: boolean;
|
||||
private readonly skillsSupport: boolean;
|
||||
@@ -3020,15 +3020,15 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
return remoteThreshold;
|
||||
}
|
||||
|
||||
async getUserCaching(): Promise<boolean | undefined> {
|
||||
getUserCaching(): boolean | undefined {
|
||||
return this.getExperimentValue<boolean>(ExperimentFlags.USER_CACHING);
|
||||
}
|
||||
|
||||
async getPlanModeRoutingEnabled(): Promise<boolean> {
|
||||
getPlanModeRoutingEnabled(): boolean {
|
||||
return this.planModeRoutingEnabled;
|
||||
}
|
||||
|
||||
async getNumericalRoutingEnabled(): Promise<boolean> {
|
||||
isNumericalRoutingEnabled(): boolean {
|
||||
return (
|
||||
this.getExperimentValue<boolean>(
|
||||
ExperimentFlags.ENABLE_NUMERICAL_ROUTING,
|
||||
@@ -3041,8 +3041,8 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
* If a remote threshold is provided and within range (0-100), it is returned.
|
||||
* Otherwise, the default threshold (90) is returned.
|
||||
*/
|
||||
async getResolvedClassifierThreshold(): Promise<number> {
|
||||
const remoteValue = await this.getClassifierThreshold();
|
||||
getResolvedClassifierThreshold(): number {
|
||||
const remoteValue = this.getClassifierThreshold();
|
||||
const defaultValue = 90;
|
||||
|
||||
if (
|
||||
@@ -3057,13 +3057,13 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
return defaultValue;
|
||||
}
|
||||
|
||||
async getClassifierThreshold(): Promise<number | undefined> {
|
||||
getClassifierThreshold(): number | undefined {
|
||||
return this.getExperimentValue<number>(
|
||||
ExperimentFlags.CLASSIFIER_THRESHOLD,
|
||||
);
|
||||
}
|
||||
|
||||
async getBannerTextNoCapacityIssues(): Promise<string> {
|
||||
getBannerTextNoCapacityIssues(): string {
|
||||
return (
|
||||
this.getExperimentValue<string>(
|
||||
ExperimentFlags.BANNER_TEXT_NO_CAPACITY_ISSUES,
|
||||
@@ -3071,7 +3071,7 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
);
|
||||
}
|
||||
|
||||
async getBannerTextCapacityIssues(): Promise<string> {
|
||||
getBannerTextCapacityIssues(): string {
|
||||
return (
|
||||
this.getExperimentValue<string>(
|
||||
ExperimentFlags.BANNER_TEXT_CAPACITY_ISSUES,
|
||||
@@ -3738,6 +3738,18 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
return ExperimentMetadata[flagId]?.defaultValue as unknown as T;
|
||||
}
|
||||
|
||||
/**
|
||||
* Updates experimental settings.
|
||||
*/
|
||||
updateExperimentalSettings(settings: Record<string, unknown>): void {
|
||||
// Only update if settings have actually changed to avoid unnecessary re-initialization logic
|
||||
// if we add any in the future.
|
||||
this.experimentalSettings = {
|
||||
...this.experimentalSettings,
|
||||
...settings,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Set experiments configuration
|
||||
*/
|
||||
|
||||
@@ -51,15 +51,12 @@ describe('ModelRouterService', () => {
|
||||
mockBaseLlmClient = {} as BaseLlmClient;
|
||||
mockLocalLiteRtLmClient = {} as LocalLiteRtLmClient;
|
||||
vi.spyOn(mockConfig, 'getBaseLlmClient').mockReturnValue(mockBaseLlmClient);
|
||||
vi.spyOn(mockConfig, 'getLocalLiteRtLmClient').mockReturnValue(
|
||||
mockLocalLiteRtLmClient,
|
||||
);
|
||||
vi.spyOn(mockConfig, 'getNumericalRoutingEnabled').mockResolvedValue(true);
|
||||
vi.spyOn(mockConfig, 'getResolvedClassifierThreshold').mockResolvedValue(
|
||||
90,
|
||||
);
|
||||
vi.spyOn(mockConfig, 'getClassifierThreshold').mockResolvedValue(undefined);
|
||||
vi.spyOn(mockConfig, 'getGemmaModelRouterSettings').mockReturnValue({
|
||||
vi.spyOn(mockConfig, 'getLocalLiteRtLmClient').mockReturnValue(
|
||||
mockLocalLiteRtLmClient,
|
||||
);
|
||||
vi.spyOn(mockConfig, 'isNumericalRoutingEnabled').mockReturnValue(true);
|
||||
vi.spyOn(mockConfig, 'getResolvedClassifierThreshold').mockReturnValue(90);
|
||||
vi.spyOn(mockConfig, 'getClassifierThreshold').mockReturnValue(undefined); vi.spyOn(mockConfig, 'getGemmaModelRouterSettings').mockReturnValue({
|
||||
enabled: false,
|
||||
classifier: {
|
||||
host: 'http://localhost:1234',
|
||||
|
||||
@@ -76,10 +76,8 @@ export class ModelRouterService {
|
||||
const startTime = Date.now();
|
||||
let decision: RoutingDecision;
|
||||
|
||||
const [enableNumericalRouting, thresholdValue] = await Promise.all([
|
||||
this.config.getNumericalRoutingEnabled(),
|
||||
this.config.getResolvedClassifierThreshold(),
|
||||
]);
|
||||
const enableNumericalRouting = this.config.isNumericalRoutingEnabled();
|
||||
const thresholdValue = this.config.getResolvedClassifierThreshold();
|
||||
const classifierThreshold = String(thresholdValue);
|
||||
|
||||
let failed = false;
|
||||
|
||||
@@ -57,7 +57,7 @@ describe('ClassifierStrategy', () => {
|
||||
getResolvedConfig: vi.fn().mockReturnValue(mockResolvedConfig),
|
||||
},
|
||||
getModel: vi.fn().mockReturnValue(DEFAULT_GEMINI_MODEL_AUTO),
|
||||
getNumericalRoutingEnabled: vi.fn().mockResolvedValue(false),
|
||||
isNumericalRoutingEnabled: vi.fn().mockReturnValue(false),
|
||||
getGemini31Launched: vi.fn().mockResolvedValue(false),
|
||||
getGemini31FlashLiteLaunched: vi.fn().mockResolvedValue(false),
|
||||
getUseCustomToolModel: vi.fn().mockImplementation(async () => {
|
||||
@@ -78,7 +78,7 @@ describe('ClassifierStrategy', () => {
|
||||
});
|
||||
|
||||
it('should return null if numerical routing is enabled and model is Gemini 3', async () => {
|
||||
vi.mocked(mockConfig.getNumericalRoutingEnabled).mockResolvedValue(true);
|
||||
vi.mocked(mockConfig.isNumericalRoutingEnabled).mockReturnValue(true);
|
||||
vi.mocked(mockConfig.getModel).mockReturnValue(PREVIEW_GEMINI_MODEL_AUTO);
|
||||
|
||||
const decision = await strategy.route(
|
||||
@@ -93,7 +93,7 @@ describe('ClassifierStrategy', () => {
|
||||
});
|
||||
|
||||
it('should NOT return null if numerical routing is enabled but model is NOT Gemini 3', async () => {
|
||||
vi.mocked(mockConfig.getNumericalRoutingEnabled).mockResolvedValue(true);
|
||||
vi.mocked(mockConfig.isNumericalRoutingEnabled).mockReturnValue(true);
|
||||
vi.mocked(mockConfig.getModel).mockReturnValue(DEFAULT_GEMINI_MODEL_AUTO);
|
||||
vi.mocked(mockBaseLlmClient.generateJson).mockResolvedValue({
|
||||
reasoning: 'test',
|
||||
|
||||
@@ -137,10 +137,7 @@ export class ClassifierStrategy implements RoutingStrategy {
|
||||
const startTime = Date.now();
|
||||
try {
|
||||
const model = context.requestedModel ?? config.getModel();
|
||||
if (
|
||||
(await config.getNumericalRoutingEnabled()) &&
|
||||
isGemini3Model(model, config)
|
||||
) {
|
||||
if (config.isNumericalRoutingEnabled() && isGemini3Model(model, config)) {
|
||||
return null;
|
||||
}
|
||||
|
||||
|
||||
@@ -55,9 +55,9 @@ describe('NumericalClassifierStrategy', () => {
|
||||
},
|
||||
getModel: vi.fn().mockReturnValue(PREVIEW_GEMINI_MODEL_AUTO),
|
||||
getSessionId: vi.fn().mockReturnValue('control-group-id'), // Default to Control Group (Hash 71 >= 50)
|
||||
getNumericalRoutingEnabled: vi.fn().mockResolvedValue(true),
|
||||
getResolvedClassifierThreshold: vi.fn().mockResolvedValue(90),
|
||||
getClassifierThreshold: vi.fn().mockResolvedValue(undefined),
|
||||
isNumericalRoutingEnabled: vi.fn().mockReturnValue(true),
|
||||
getResolvedClassifierThreshold: vi.fn().mockReturnValue(90),
|
||||
getClassifierThreshold: vi.fn().mockReturnValue(undefined),
|
||||
getGemini31Launched: vi.fn().mockResolvedValue(false),
|
||||
getGemini31FlashLiteLaunched: vi.fn().mockResolvedValue(false),
|
||||
getUseCustomToolModel: vi.fn().mockImplementation(async () => {
|
||||
@@ -82,7 +82,7 @@ describe('NumericalClassifierStrategy', () => {
|
||||
});
|
||||
|
||||
it('should return null if numerical routing is disabled', async () => {
|
||||
vi.mocked(mockConfig.getNumericalRoutingEnabled).mockResolvedValue(false);
|
||||
vi.mocked(mockConfig.isNumericalRoutingEnabled).mockReturnValue(false);
|
||||
|
||||
const decision = await strategy.route(
|
||||
mockContext,
|
||||
|
||||
@@ -105,7 +105,7 @@ export class NumericalClassifierStrategy implements RoutingStrategy {
|
||||
const startTime = Date.now();
|
||||
try {
|
||||
const model = context.requestedModel ?? config.getModel();
|
||||
if (!(await config.getNumericalRoutingEnabled())) {
|
||||
if (!config.isNumericalRoutingEnabled()) {
|
||||
return null;
|
||||
}
|
||||
|
||||
@@ -187,8 +187,8 @@ export class NumericalClassifierStrategy implements RoutingStrategy {
|
||||
groupLabel: string;
|
||||
modelAlias: typeof FLASH_MODEL | typeof PRO_MODEL;
|
||||
}> {
|
||||
const threshold = await config.getResolvedClassifierThreshold();
|
||||
const remoteThresholdValue = await config.getClassifierThreshold();
|
||||
const threshold = config.getResolvedClassifierThreshold();
|
||||
const remoteThresholdValue = config.getClassifierThreshold();
|
||||
|
||||
let groupLabel: string;
|
||||
if (threshold === remoteThresholdValue) {
|
||||
|
||||
Reference in New Issue
Block a user