Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions packages/core/src/core/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@ ${JSON.stringify(
// Assert
expect(ideContextStore.get).toHaveBeenCalled();
expect(mockTurnRunFn).toHaveBeenCalledWith(
{ model: 'default-routed-model' },
{ model: 'default-routed-model', isChatModel: true },
initialRequest,
expect.any(AbortSignal),
undefined,
Expand Down Expand Up @@ -1722,7 +1722,7 @@ ${JSON.stringify(
expect(mockConfig.getModelRouterService).toHaveBeenCalled();
expect(mockRouterService.route).toHaveBeenCalled();
expect(mockTurnRunFn).toHaveBeenCalledWith(
{ model: 'routed-model' },
{ model: 'routed-model', isChatModel: true },
[{ text: 'Hi' }],
expect.any(AbortSignal),
undefined,
Expand All @@ -1740,7 +1740,7 @@ ${JSON.stringify(

expect(mockRouterService.route).toHaveBeenCalledTimes(1);
expect(mockTurnRunFn).toHaveBeenCalledWith(
{ model: 'routed-model' },
{ model: 'routed-model', isChatModel: true },
[{ text: 'Hi' }],
expect.any(AbortSignal),
undefined,
Expand All @@ -1758,7 +1758,7 @@ ${JSON.stringify(
expect(mockRouterService.route).toHaveBeenCalledTimes(1);
// Should stick to the first model
expect(mockTurnRunFn).toHaveBeenCalledWith(
{ model: 'routed-model' },
{ model: 'routed-model', isChatModel: true },
[{ text: 'Continue' }],
expect.any(AbortSignal),
undefined,
Expand All @@ -1776,7 +1776,7 @@ ${JSON.stringify(

expect(mockRouterService.route).toHaveBeenCalledTimes(1);
expect(mockTurnRunFn).toHaveBeenCalledWith(
{ model: 'routed-model' },
{ model: 'routed-model', isChatModel: true },
[{ text: 'Hi' }],
expect.any(AbortSignal),
undefined,
Expand All @@ -1798,7 +1798,7 @@ ${JSON.stringify(
expect(mockRouterService.route).toHaveBeenCalledTimes(2);
// Should use the newly routed model
expect(mockTurnRunFn).toHaveBeenCalledWith(
{ model: 'new-routed-model' },
{ model: 'new-routed-model', isChatModel: true },
[{ text: 'A new topic' }],
expect.any(AbortSignal),
undefined,
Expand Down Expand Up @@ -1826,7 +1826,7 @@ ${JSON.stringify(
expect(mockRouterService.route).toHaveBeenCalledTimes(1);
expect(mockTurnRunFn).toHaveBeenNthCalledWith(
1,
{ model: 'original-model' },
{ model: 'original-model', isChatModel: true },
[{ text: 'Hi' }],
expect.any(AbortSignal),
undefined,
Expand All @@ -1849,7 +1849,7 @@ ${JSON.stringify(
expect(mockRouterService.route).toHaveBeenCalledTimes(2);
expect(mockTurnRunFn).toHaveBeenNthCalledWith(
2,
{ model: 'fallback-model' },
{ model: 'fallback-model', isChatModel: true },
[{ text: 'Continue' }],
expect.any(AbortSignal),
undefined,
Expand Down Expand Up @@ -1935,7 +1935,7 @@ ${JSON.stringify(
// First call with original request
expect(mockTurnRunFn).toHaveBeenNthCalledWith(
1,
{ model: 'default-routed-model' },
{ model: 'default-routed-model', isChatModel: true },
initialRequest,
expect.any(AbortSignal),
undefined,
Expand All @@ -1944,7 +1944,7 @@ ${JSON.stringify(
// Second call with "Please continue."
expect(mockTurnRunFn).toHaveBeenNthCalledWith(
2,
{ model: 'default-routed-model' },
{ model: 'default-routed-model', isChatModel: true },
[{ text: 'System: Please continue.' }],
expect.any(AbortSignal),
undefined,
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/core/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,10 @@ export class GeminiClient {
}

// availability logic
const modelConfigKey: ModelConfigKey = { model: modelToUse };
const modelConfigKey: ModelConfigKey = {
model: modelToUse,
isChatModel: true,
};
const { model: finalModel } = applyModelSelection(
this.config,
modelConfigKey,
Expand Down
65 changes: 65 additions & 0 deletions packages/core/src/services/modelConfigService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,71 @@ describe('ModelConfigService', () => {
});
});

describe('fallback behavior', () => {
it('should fallback to chat-base if the requested model is completely unknown', () => {
const config: ModelConfigServiceConfig = {
aliases: {
'chat-base': {
modelConfig: {
model: 'default-fallback-model',
generateContentConfig: {
temperature: 0.99,
},
},
},
},
};
const service = new ModelConfigService(config);
const resolved = service.getResolvedConfig({
model: 'my-custom-model',
isChatModel: true,
});

// It preserves the requested model name, but inherits the config from chat-base
expect(resolved.model).toBe('my-custom-model');
expect(resolved.generateContentConfig).toEqual({
temperature: 0.99,
});
});

it('should return empty config if requested model is unknown and chat-base is not defined', () => {
const config: ModelConfigServiceConfig = {
aliases: {},
};
const service = new ModelConfigService(config);
const resolved = service.getResolvedConfig({
model: 'my-custom-model',
isChatModel: true,
});

expect(resolved.model).toBe('my-custom-model');
expect(resolved.generateContentConfig).toEqual({});
});

it('should NOT fallback to chat-base if the requested model is completely unknown but isChatModel is false', () => {
const config: ModelConfigServiceConfig = {
aliases: {
'chat-base': {
modelConfig: {
model: 'default-fallback-model',
generateContentConfig: {
temperature: 0.99,
},
},
},
},
};
const service = new ModelConfigService(config);
const resolved = service.getResolvedConfig({
model: 'my-custom-model',
isChatModel: false,
});

expect(resolved.model).toBe('my-custom-model');
expect(resolved.generateContentConfig).toEqual({});
});
});

describe('unrecognized models', () => {
it('should apply overrides to unrecognized model names', () => {
const unregisteredModelName = 'my-unregistered-model-v1';
Expand Down
21 changes: 21 additions & 0 deletions packages/core/src/services/modelConfigService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ export interface ModelConfigKey {
// This allows overrides to specify different settings (e.g., higher temperature)
// specifically for retry scenarios.
isRetry?: boolean;

// Indicates whether this request originates from the primary interactive chat model.
// Enables the default fallback configuration to `chat-base` when unknown.
isChatModel?: boolean;
}

export interface ModelConfig {
Expand Down Expand Up @@ -122,6 +126,7 @@ export class ModelConfigService {
const { aliasChain, baseModel, resolvedConfig } = this.resolveAliasChain(
context.model,
allAliases,
context.isChatModel,
);

const modelToLevel = this.buildModelLevelMap(aliasChain, baseModel);
Expand Down Expand Up @@ -159,6 +164,7 @@ export class ModelConfigService {
private resolveAliasChain(
requestedModel: string,
allAliases: Record<string, ModelConfigAlias>,
isChatModel?: boolean,
): {
aliasChain: string[];
baseModel: string | undefined;
Expand Down Expand Up @@ -206,6 +212,21 @@ export class ModelConfigService {
};
}

if (isChatModel) {
const fallbackAlias = 'chat-base';
if (allAliases[fallbackAlias]) {
const fallbackResolution = this.resolveAliasChain(
fallbackAlias,
allAliases,
);
return {
aliasChain: [...fallbackResolution.aliasChain, requestedModel],
baseModel: requestedModel,
resolvedConfig: fallbackResolution.resolvedConfig,
};
Comment on lines +222 to +226
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This implementation correctly preserves the requested model name, which is good. However, by setting baseModel: requestedModel, it prevents any overrides that target the actual base model of chat-base (e.g., gemini-pro) from being applied. This can lead to surprising behavior where an unknown model doesn't fully inherit the behavior of chat-base.

For example, if chat-base uses gemini-pro and there is an override defined for gemini-pro, that override will not be applied when resolving an unrecognized model via this fallback logic.

To fix this, fallbackResolution.baseModel should be used for override matching, while still ensuring the final resolved model is requestedModel. This likely requires a more significant refactoring, for example, by separating the model used for override resolution from the final resolved model name within internalGetResolvedConfig.

}
}

return {
aliasChain: [requestedModel],
baseModel: requestedModel,
Expand Down
Loading