mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-11 06:31:01 -07:00
bug(core): fix issue with overrides to bases. (#15255)
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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<string, ModelConfigAlias> = {};
|
||||
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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string, ModelConfigAlias>,
|
||||
visited = new Set<string>(),
|
||||
): 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<typeof match> => 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<string, ModelConfigAlias>,
|
||||
): {
|
||||
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<string>();
|
||||
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<string, number> {
|
||||
const modelToLevel = new Map<string, number>();
|
||||
// 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<string, number>,
|
||||
): 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<typeof m> => 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);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user