From eb75f59a96e48e4da5bd995be5f27da8d1ae4561 Mon Sep 17 00:00:00 2001 From: joshualitt Date: Thu, 8 Jan 2026 06:59:58 -0800 Subject: [PATCH] bug(core): fix issue with overrides to bases. (#15255) --- .../services/modelConfig.integration.test.ts | 4 +- .../src/services/modelConfigService.test.ts | 65 ++++- .../core/src/services/modelConfigService.ts | 273 +++++++++++------- 3 files changed, 234 insertions(+), 108 deletions(-) diff --git a/packages/core/src/services/modelConfig.integration.test.ts b/packages/core/src/services/modelConfig.integration.test.ts index c6daf962b6..2ed2cb47af 100644 --- a/packages/core/src/services/modelConfig.integration.test.ts +++ b/packages/core/src/services/modelConfig.integration.test.ts @@ -141,7 +141,7 @@ describe('ModelConfigService Integration', () => { // No agent specified, so it should match core agent-specific rules }); - expect(resolved.model).toBe('gemini-1.5-flash-latest'); // from alias + expect(resolved.model).toBe('gemini-1.5-pro-latest'); // now overridden by 'base' expect(resolved.generateContentConfig).toEqual({ topP: 0.95, // from base topK: 64, // from base @@ -171,7 +171,7 @@ describe('ModelConfigService Integration', () => { overrideScope: 'core', }); - expect(resolved.model).toBe('gemini-1.5-flash-latest'); + expect(resolved.model).toBe('gemini-1.5-pro-latest'); // now overridden by 'base' expect(resolved.generateContentConfig).toEqual({ // Inherited from 'base' topP: 0.95, diff --git a/packages/core/src/services/modelConfigService.test.ts b/packages/core/src/services/modelConfigService.test.ts index ee6cd09f40..c9ddda2e2b 100644 --- a/packages/core/src/services/modelConfigService.test.ts +++ b/packages/core/src/services/modelConfigService.test.ts @@ -5,7 +5,10 @@ */ import { describe, it, expect } from 'vitest'; -import type { ModelConfigServiceConfig } from './modelConfigService.js'; +import type { + ModelConfigAlias, + ModelConfigServiceConfig, +} from './modelConfigService.js'; import { ModelConfigService } from './modelConfigService.js'; describe('ModelConfigService', () => { @@ -470,6 +473,21 @@ describe('ModelConfigService', () => { 'Alias "non-existent" not found.', ); }); + + it('should throw an error if the alias chain is too deep', () => { + const aliases: Record = {}; + for (let i = 0; i < 101; i++) { + aliases[`alias-${i}`] = { + extends: i === 100 ? undefined : `alias-${i + 1}`, + modelConfig: i === 100 ? { model: 'gemini-pro' } : {}, + }; + } + const config: ModelConfigServiceConfig = { aliases }; + const service = new ModelConfigService(config); + expect(() => service.getResolvedConfig({ model: 'alias-0' })).toThrow( + 'Alias inheritance chain exceeded maximum depth of 100.', + ); + }); }); describe('deep merging', () => { @@ -889,5 +907,50 @@ describe('ModelConfigService', () => { }); expect(retry.generateContentConfig.temperature).toBe(1.0); }); + + it('should apply overrides to parents in the alias hierarchy', () => { + const config: ModelConfigServiceConfig = { + aliases: { + 'base-alias': { + modelConfig: { + model: 'gemini-test', + generateContentConfig: { + temperature: 0.5, + }, + }, + }, + 'child-alias': { + extends: 'base-alias', + modelConfig: { + generateContentConfig: { + topP: 0.9, + }, + }, + }, + }, + overrides: [ + { + match: { model: 'base-alias', isRetry: true }, + modelConfig: { + generateContentConfig: { + temperature: 1.0, + }, + }, + }, + ], + }; + const service = new ModelConfigService(config); + + // Normal request + const normal = service.getResolvedConfig({ model: 'child-alias' }); + expect(normal.generateContentConfig.temperature).toBe(0.5); + + // Retry request - should match override on parent + const retry = service.getResolvedConfig({ + model: 'child-alias', + isRetry: true, + }); + expect(retry.generateContentConfig.temperature).toBe(1.0); + }); }); }); diff --git a/packages/core/src/services/modelConfigService.ts b/packages/core/src/services/modelConfigService.ts index 6fb712243c..0ec6d77ffb 100644 --- a/packages/core/src/services/modelConfigService.ts +++ b/packages/core/src/services/modelConfigService.ts @@ -54,6 +54,8 @@ export interface ModelConfigServiceConfig { customOverrides?: ModelConfigOverride[]; } +const MAX_ALIAS_CHAIN_DEPTH = 100; + export type ResolvedModelConfig = _ResolvedModelConfig & { readonly _brand: unique symbol; }; @@ -78,130 +80,68 @@ export class ModelConfigService { this.runtimeOverrides.push(override); } - private resolveAlias( - aliasName: string, - aliases: Record, - visited = new Set(), - ): ModelConfigAlias { - if (visited.has(aliasName)) { - throw new Error( - `Circular alias dependency: ${[...visited, aliasName].join(' -> ')}`, - ); - } - visited.add(aliasName); - - const alias = aliases[aliasName]; - if (!alias) { - throw new Error(`Alias "${aliasName}" not found.`); - } - - if (!alias.extends) { - return alias; - } - - const baseAlias = this.resolveAlias(alias.extends, aliases, visited); - - return { - modelConfig: { - model: alias.modelConfig.model ?? baseAlias.modelConfig.model, - generateContentConfig: this.deepMerge( - baseAlias.modelConfig.generateContentConfig, - alias.modelConfig.generateContentConfig, - ), - }, - }; - } - + /** + * Resolves a model configuration by merging settings from aliases and applying overrides. + * + * The resolution follows a linear application pipeline: + * + * 1. Alias Chain Resolution: + * Builds the inheritance chain from root to leaf. Configurations are merged starting from + * the root, so that children naturally override parents. + * + * 2. Override Level Assignment: + * Overrides are matched against the hierarchy and assigned a "Level" for application: + * - Level 0: Broad matches (Global or Resolved Model name). + * - Level 1..N: Hierarchy matches (from Root-most alias to Leaf-most alias). + * + * 3. Precedence & Application: + * Overrides are applied in order of their Level (ASC), then Specificity (ASC), then + * Configuration Order (ASC). This ensures that more targeted and "deeper" rules + * naturally layer on top of broader ones. + * + * 4. Orthogonality: + * All fields (including 'model') are treated equally. A more specific or deeper override + * can freely change any setting, including the target model name. + */ private internalGetResolvedConfig(context: ModelConfigKey): { model: string | undefined; generateContentConfig: GenerateContentConfig; } { - const config = this.config || {}; const { aliases = {}, customAliases = {}, overrides = [], customOverrides = [], - } = config; + } = this.config || {}; const allAliases = { ...aliases, ...customAliases, ...this.runtimeAliases, }; + + const { + aliasChain, + baseModel: initialBaseModel, + resolvedConfig: initialResolvedConfig, + } = this.resolveAliasChain(context.model, allAliases); + + let baseModel = initialBaseModel; + let resolvedConfig = initialResolvedConfig; + + const modelToLevel = this.buildModelLevelMap(aliasChain, baseModel); const allOverrides = [ ...overrides, ...customOverrides, ...this.runtimeOverrides, ]; - let baseModel: string | undefined = context.model; - let resolvedConfig: GenerateContentConfig = {}; + const matches = this.findMatchingOverrides( + allOverrides, + context, + modelToLevel, + ); - // Step 1: Alias Resolution - if (allAliases[context.model]) { - const resolvedAlias = this.resolveAlias(context.model, allAliases); - baseModel = resolvedAlias.modelConfig.model; // This can now be undefined - resolvedConfig = this.deepMerge( - resolvedConfig, - resolvedAlias.modelConfig.generateContentConfig, - ); - } + this.sortOverrides(matches); - // If an alias was used but didn't resolve to a model, `baseModel` is undefined. - // We still need a model for matching overrides. We'll use the original alias name - // for matching if no model is resolved yet. - const modelForMatching = baseModel ?? context.model; - - const finalContext = { - ...context, - model: modelForMatching, - }; - - // Step 2: Override Application - const matches = allOverrides - .map((override, index) => { - const matchEntries = Object.entries(override.match); - if (matchEntries.length === 0) { - return null; - } - - const isMatch = matchEntries.every(([key, value]) => { - if (key === 'model') { - return value === context.model || value === finalContext.model; - } - if (key === 'overrideScope' && value === 'core') { - // The 'core' overrideScope is special. It should match if the - // overrideScope is explicitly 'core' or if the overrideScope - // is not specified. - return context.overrideScope === 'core' || !context.overrideScope; - } - return finalContext[key as keyof ModelConfigKey] === value; - }); - - if (isMatch) { - return { - specificity: matchEntries.length, - modelConfig: override.modelConfig, - index, - }; - } - return null; - }) - .filter((match): match is NonNullable => match !== null); - - // The override application logic is designed to be both simple and powerful. - // By first sorting all matching overrides by specificity (and then by their - // original order as a tie-breaker), we ensure that as we merge the `config` - // objects, the settings from the most specific rules are applied last, - // correctly overwriting any values from broader, less-specific rules. - // This achieves a per-property override effect without complex per-property logic. - matches.sort((a, b) => { - if (a.specificity !== b.specificity) { - return a.specificity - b.specificity; - } - return a.index - b.index; - }); - - // Apply matching overrides for (const match of matches) { if (match.modelConfig.model) { baseModel = match.modelConfig.model; @@ -214,12 +154,135 @@ export class ModelConfigService { } } + return { model: baseModel, generateContentConfig: resolvedConfig }; + } + + private resolveAliasChain( + requestedModel: string, + allAliases: Record, + ): { + aliasChain: string[]; + baseModel: string | undefined; + resolvedConfig: GenerateContentConfig; + } { + let baseModel: string | undefined = undefined; + let resolvedConfig: GenerateContentConfig = {}; + const aliasChain: string[] = []; + + if (allAliases[requestedModel]) { + let current: string | undefined = requestedModel; + const visited = new Set(); + while (current) { + const alias: ModelConfigAlias = allAliases[current]; + if (!alias) { + throw new Error(`Alias "${current}" not found.`); + } + if (visited.size >= MAX_ALIAS_CHAIN_DEPTH) { + throw new Error( + `Alias inheritance chain exceeded maximum depth of ${MAX_ALIAS_CHAIN_DEPTH}.`, + ); + } + if (visited.has(current)) { + throw new Error( + `Circular alias dependency: ${[...visited, current].join(' -> ')}`, + ); + } + visited.add(current); + aliasChain.push(current); + current = alias.extends; + } + + // Root-to-Leaf chain for merging and level assignment. + const reversedChain = [...aliasChain].reverse(); + for (const aliasName of reversedChain) { + const alias = allAliases[aliasName]; + if (alias.modelConfig.model) { + baseModel = alias.modelConfig.model; + } + resolvedConfig = this.deepMerge( + resolvedConfig, + alias.modelConfig.generateContentConfig, + ); + } + return { aliasChain: reversedChain, baseModel, resolvedConfig }; + } + return { - model: baseModel, - generateContentConfig: resolvedConfig, + aliasChain: [requestedModel], + baseModel: requestedModel, + resolvedConfig: {}, }; } + private buildModelLevelMap( + aliasChain: string[], + baseModel: string | undefined, + ): Map { + const modelToLevel = new Map(); + // Global and Model name are both level 0. + if (baseModel) { + modelToLevel.set(baseModel, 0); + } + // Alias chain starts at level 1. + aliasChain.forEach((name, i) => modelToLevel.set(name, i + 1)); + return modelToLevel; + } + + private findMatchingOverrides( + overrides: ModelConfigOverride[], + context: ModelConfigKey, + modelToLevel: Map, + ): Array<{ + specificity: number; + level: number; + modelConfig: ModelConfig; + index: number; + }> { + return overrides + .map((override, index) => { + const matchEntries = Object.entries(override.match); + if (matchEntries.length === 0) return null; + + let matchedLevel = 0; // Default to Global + const isMatch = matchEntries.every(([key, value]) => { + if (key === 'model') { + const level = modelToLevel.get(value as string); + if (level === undefined) return false; + matchedLevel = level; + return true; + } + if (key === 'overrideScope' && value === 'core') { + return context.overrideScope === 'core' || !context.overrideScope; + } + return context[key as keyof ModelConfigKey] === value; + }); + + return isMatch + ? { + specificity: matchEntries.length, + level: matchedLevel, + modelConfig: override.modelConfig, + index, + } + : null; + }) + .filter((m): m is NonNullable => m !== null); + } + + private sortOverrides( + matches: Array<{ specificity: number; level: number; index: number }>, + ): void { + matches.sort((a, b) => { + if (a.level !== b.level) { + return a.level - b.level; + } + if (a.specificity !== b.specificity) { + return a.specificity - b.specificity; + } + return a.index - b.index; + }); + } + getResolvedConfig(context: ModelConfigKey): ResolvedModelConfig { const resolved = this.internalGetResolvedConfig(context);