diff --git a/.gemini/skills/experimentation/SKILL.md b/.gemini/skills/experimentation/SKILL.md index 00df4c0e33..a195221126 100644 --- a/.gemini/skills/experimentation/SKILL.md +++ b/.gemini/skills/experimentation/SKILL.md @@ -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(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(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(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 diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 3f5c2dfb7d..e62f856c45 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -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, diff --git a/packages/cli/src/test-utils/mockConfig.ts b/packages/cli/src/test-utils/mockConfig.ts index 6561ac1db0..0cf4112d58 100644 --- a/packages/cli/src/test-utils/mockConfig.ts +++ b/packages/cli/src/test-utils/mockConfig.ts @@ -120,7 +120,7 @@ export const createMockConfig = (overrides: Partial = {}): 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(''), diff --git a/packages/cli/src/ui/commands/experimentCommand.test.ts b/packages/cli/src/ui/commands/experimentCommand.test.ts index 9e413cc2fc..11d495e0b9 100644 --- a/packages/cli/src/ui/commands/experimentCommand.test.ts +++ b/packages/cli/src/ui/commands/experimentCommand.test.ts @@ -18,6 +18,7 @@ describe('experimentCommand', () => { services: { config: { getExperimentValue: vi.fn(), + updateExperimentalSettings: vi.fn(), }, settings: { merged: { diff --git a/packages/cli/src/ui/commands/experimentCommand.ts b/packages/cli/src/ui/commands/experimentCommand.ts index 2a6aca44b9..ca7b64256e 100644 --- a/packages/cli/src/ui/commands/experimentCommand.ts +++ b/packages/cli/src/ui/commands/experimentCommand.ts @@ -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) || {}), }; 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) || {}), }; @@ -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, diff --git a/packages/core/src/code_assist/experiments/flagNames.ts b/packages/core/src/code_assist/experiments/flagNames.ts index 7333dac573..7e3931f0d9 100644 --- a/packages/core/src/code_assist/experiments/flagNames.ts +++ b/packages/core/src/code_assist/experiments/flagNames.ts @@ -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)[constantName]; } diff --git a/packages/core/src/config/config.cli_override.test.ts b/packages/core/src/config/config.cli_override.test.ts new file mode 100644 index 0000000000..04524b1d62 --- /dev/null +++ b/packages/core/src/config/config.cli_override.test.ts @@ -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); + }); +}); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index dadbda15d3..a3c0549c23 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -942,7 +942,7 @@ export class Config implements McpContext, AgentLoopContext { private readonly enableAgents: boolean; private agents: AgentSettings; - private readonly experimentalSettings: Record; + private experimentalSettings: Record; private readonly experimentalCliArgs: Record; private readonly enableEventDrivenScheduler: boolean; private readonly skillsSupport: boolean; @@ -3020,15 +3020,15 @@ export class Config implements McpContext, AgentLoopContext { return remoteThreshold; } - async getUserCaching(): Promise { + getUserCaching(): boolean | undefined { return this.getExperimentValue(ExperimentFlags.USER_CACHING); } - async getPlanModeRoutingEnabled(): Promise { + getPlanModeRoutingEnabled(): boolean { return this.planModeRoutingEnabled; } - async getNumericalRoutingEnabled(): Promise { + isNumericalRoutingEnabled(): boolean { return ( this.getExperimentValue( 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 { - 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 { + getClassifierThreshold(): number | undefined { return this.getExperimentValue( ExperimentFlags.CLASSIFIER_THRESHOLD, ); } - async getBannerTextNoCapacityIssues(): Promise { + getBannerTextNoCapacityIssues(): string { return ( this.getExperimentValue( ExperimentFlags.BANNER_TEXT_NO_CAPACITY_ISSUES, @@ -3071,7 +3071,7 @@ export class Config implements McpContext, AgentLoopContext { ); } - async getBannerTextCapacityIssues(): Promise { + getBannerTextCapacityIssues(): string { return ( this.getExperimentValue( 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): 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 */ diff --git a/packages/core/src/routing/modelRouterService.test.ts b/packages/core/src/routing/modelRouterService.test.ts index 4e0c32c62f..348637c36a 100644 --- a/packages/core/src/routing/modelRouterService.test.ts +++ b/packages/core/src/routing/modelRouterService.test.ts @@ -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', diff --git a/packages/core/src/routing/modelRouterService.ts b/packages/core/src/routing/modelRouterService.ts index a62deacd31..34553fcc1f 100644 --- a/packages/core/src/routing/modelRouterService.ts +++ b/packages/core/src/routing/modelRouterService.ts @@ -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; diff --git a/packages/core/src/routing/strategies/classifierStrategy.test.ts b/packages/core/src/routing/strategies/classifierStrategy.test.ts index 373da6f144..506be0e795 100644 --- a/packages/core/src/routing/strategies/classifierStrategy.test.ts +++ b/packages/core/src/routing/strategies/classifierStrategy.test.ts @@ -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', diff --git a/packages/core/src/routing/strategies/classifierStrategy.ts b/packages/core/src/routing/strategies/classifierStrategy.ts index 1dd09f4596..1fa4c29ca1 100644 --- a/packages/core/src/routing/strategies/classifierStrategy.ts +++ b/packages/core/src/routing/strategies/classifierStrategy.ts @@ -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; } diff --git a/packages/core/src/routing/strategies/numericalClassifierStrategy.test.ts b/packages/core/src/routing/strategies/numericalClassifierStrategy.test.ts index dcfdff786b..7db2aa7c90 100644 --- a/packages/core/src/routing/strategies/numericalClassifierStrategy.test.ts +++ b/packages/core/src/routing/strategies/numericalClassifierStrategy.test.ts @@ -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, diff --git a/packages/core/src/routing/strategies/numericalClassifierStrategy.ts b/packages/core/src/routing/strategies/numericalClassifierStrategy.ts index 8bcfb3da67..e3cb2eefbc 100644 --- a/packages/core/src/routing/strategies/numericalClassifierStrategy.ts +++ b/packages/core/src/routing/strategies/numericalClassifierStrategy.ts @@ -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) {