diff --git a/src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts b/src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts index 4c589de5720ea..b5f4754746de9 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts @@ -1588,14 +1588,21 @@ export class ChatInputPart extends Disposable implements IHistoryNavigationWidge /** * Get the chat session type for the current session, if any. + * + * Once a real session exists, the session resource is the authoritative + * source for which models are valid. The picker delegate only describes the + * welcome/new-session selection, which may not match the session that was + * ultimately created (e.g. an agent-host pick that fell back to an + * in-process `local` session). Preferring the delegate in that case lets an + * agent-host model leak into a local session's pool, so we only consult the + * delegate when there is no session yet (the welcome view has no view model). */ private getCurrentSessionType(): string | undefined { - const delegateSessionType = this.options.sessionTypePickerDelegate?.getActiveSessionProvider?.(); - if (delegateSessionType) { - return delegateSessionType; - } const sessionResource = this._widget?.viewModel?.model.sessionResource; - return sessionResource ? getChatSessionType(sessionResource) : undefined; + if (sessionResource) { + return getChatSessionType(sessionResource); + } + return this.options.sessionTypePickerDelegate?.getActiveSessionProvider?.(); } private isModelValidForCurrentSession(model: ILanguageModelChatMetadataAndIdentifier): boolean { diff --git a/src/vs/workbench/contrib/chat/browser/widget/input/chatModelConfigurationLogic.ts b/src/vs/workbench/contrib/chat/browser/widget/input/chatModelConfigurationLogic.ts index a992dfcf03714..dc49852a147f8 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/input/chatModelConfigurationLogic.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/input/chatModelConfigurationLogic.ts @@ -64,6 +64,13 @@ export function filterConfigurationToSchema( * *missing* entry (`undefined`) falls back to the profile-global value, which is * the one-time migration for setups that pre-date per-editor scoping. * + * Schema defaults are merged in every branch so a value the user never + * explicitly set (e.g. a model's default `contextSize`) is always present in the + * resolved configuration. Otherwise the model picker — which paints the schema + * default when a key is absent — would show one value while the request and the + * context-usage widget, which read the resolved configuration, fall back to the + * model's full native window. See issue #320393. + * * Distinguishing "present but empty" from "absent" is what prevents a newly * opened editor from reverting an explicit default selection back to a stale * profile-global value. See issue #320393. @@ -76,7 +83,7 @@ export function resolveModelConfiguration( if (storedEntry) { return { ...schemaDefaults, ...storedEntry }; } - return globalConfig ? { ...globalConfig } : {}; + return globalConfig ? { ...schemaDefaults, ...globalConfig } : { ...schemaDefaults }; } /** diff --git a/src/vs/workbench/contrib/chat/browser/widget/input/chatModelConfigurationStore.ts b/src/vs/workbench/contrib/chat/browser/widget/input/chatModelConfigurationStore.ts index e51a90836a9b2..f6d4f98027b29 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/input/chatModelConfigurationStore.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/input/chatModelConfigurationStore.ts @@ -39,6 +39,36 @@ export class ChatModelConfigurationStore extends Disposable implements IModelCon private readonly storageService: IStorageService, ) { super(); + + // Language model providers register asynchronously, so a model's schema + // defaults and profile-global configuration may not be available the first + // time `getModelConfiguration` is called (e.g. when the model picker or the + // context-usage widget reads it during initial layout). The empty snapshot + // resolved at that point would otherwise be memoized forever, pinning the + // model to its schema default while the request path — resolved later — + // uses the configured value. When the set of language models changes, drop + // only those poisoned snapshots so the next read recomputes against the + // now-available configuration, and notify consumers (picker, context-usage + // widget) so the stale value refreshes. + // + // This event also fires for model-configuration changes (e.g. our own global + // mirror in `setModelConfiguration`), so we must NOT clear stable snapshots: + // an entry that resolved to a non-empty value, or that is backed by a scoped + // bucket entry, is the editor's intended value and clearing it would discard + // it and cause a duplicate refresh for a single user action. Only an entry + // that resolved empty with no bucket entry can be a pre-config-load artifact. + this._register(this.languageModelsService.onDidChangeLanguageModels(() => { + if (this._overrides.size === 0) { + return; + } + const bucket = this._readBucket(); + for (const [modelId, override] of [...this._overrides]) { + if (Object.keys(override).length === 0 && bucket[modelId] === undefined) { + this._overrides.delete(modelId); + this._onDidChange.fire(modelId); + } + } + })); } /** @@ -54,17 +84,47 @@ export class ChatModelConfigurationStore extends Disposable implements IModelCon getModelConfiguration(modelId: string): IStringDictionary | undefined { let override = this._overrides.get(modelId); if (!override) { - override = resolveModelConfiguration( - this._readBucket()[modelId], - this._schemaDefaults(modelId), - this.languageModelsService.getModelConfiguration(modelId), - ); + const bucketEntry = this._readBucket()[modelId]; + const schemaDefaults = this._schemaDefaults(modelId); + const globalConfig = this.languageModelsService.getModelConfiguration(modelId); + override = resolveModelConfiguration(bucketEntry, schemaDefaults, globalConfig); this._overrides.set(modelId, override); } return Object.keys(override).length > 0 ? override : undefined; } async setModelConfiguration(modelId: string, values: IStringDictionary): Promise { + const changed = this._applyLocalModelConfiguration(modelId, values); + if (!changed) { + // No-op (e.g. re-selecting the already-current value): skip the global + // write to avoid a redundant profile-file write and the resulting + // `onDidChangeLanguageModels` event. Any real change — including + // selecting the schema default — still falls through and syncs the + // global value. + return; + } + + // Mirror the change to the profile-global model configuration. The + // per-editor bucket is the source of truth for this editor, but the + // global value is what newly created stores read as their migration + // fallback (see `getModelConfiguration`) and what other surfaces (e.g. the + // Models management view) display. Without this, changing the dropdown + // would only update the editor-scoped bucket and leave a stale global + // value behind, so a previously chosen value (e.g. the full context + // window) could get "stuck" and reappear as the apparent default whenever + // the bucket is absent. This restores the pre-#320393 behaviour where the + // picker wrote straight to the global. `setModelConfiguration` on the + // service strips values equal to their schema default, so selecting the + // default cleanly clears the global override. + await this.languageModelsService.setModelConfiguration(modelId, values); + } + + /** + * Applies the change to this editor's scoped state only (in-memory snapshot + * and persisted bucket). Returns `true` when something actually changed, so + * callers can skip propagating no-op updates to the profile-global value. + */ + private _applyLocalModelConfiguration(modelId: string, values: IStringDictionary): boolean { const schemaDefaults = this._schemaDefaults(modelId); const stored = computeStoredConfiguration(this.getModelConfiguration(modelId) ?? {}, values, schemaDefaults); const nextOverride = { ...schemaDefaults, ...stored }; @@ -74,7 +134,7 @@ export class ChatModelConfigurationStore extends Disposable implements IModelCon // storage writes and onDidChange listeners when nothing actually changes. const bucket = this._readBucket(); if (equals(this._overrides.get(modelId), nextOverride) && equals(bucket[modelId], stored)) { - return; + return false; } // In-memory snapshot keeps the full effective config (defaults + overrides). @@ -89,6 +149,7 @@ export class ChatModelConfigurationStore extends Disposable implements IModelCon this._writeBucket(bucket); this._onDidChange.fire(modelId); + return true; } getModelConfigurationActions(modelId: string): IAction[] { @@ -114,9 +175,11 @@ export class ChatModelConfigurationStore extends Disposable implements IModelCon */ restoreModelConfiguration(modelId: string, values: IStringDictionary): void { const filtered = filterConfigurationToSchema(values, this.languageModelsService.lookupLanguageModel(modelId)?.configurationSchema); - // Intentionally fire-and-forget: `setModelConfiguration` completes synchronously - // (no awaits) and the interface only returns a Promise for API symmetry. - void this.setModelConfiguration(modelId, filtered); + // Restore only seeds this editor's scoped snapshot; unlike a user-made + // change it must NOT write the profile-global value, since restoring a + // session is not an intentional reconfiguration and runs on every + // input-state sync. + this._applyLocalModelConfiguration(modelId, filtered); } /** diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/input/chatModelConfigurationLogic.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/input/chatModelConfigurationLogic.test.ts index 1ceabce8fd7c4..f4a1e260c3854 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/input/chatModelConfigurationLogic.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/input/chatModelConfigurationLogic.test.ts @@ -55,8 +55,20 @@ suite('chatModelConfigurationLogic', () => { assert.deepStrictEqual(resolved, { thinkingEffort: 'high' }); }); - test('absent entry with no global value resolves to empty', () => { - assert.deepStrictEqual(resolveModelConfiguration(undefined, defaults, undefined), {}); + test('absent entry with no global value resolves to the schema defaults', () => { + // Schema defaults must be present even when nothing is stored, so a value + // the user never explicitly set (e.g. a model's default contextSize) is + // not dropped — otherwise the request and context-usage widget fall back + // to the model's full native window while the picker shows the default. + assert.deepStrictEqual(resolveModelConfiguration(undefined, defaults, undefined), { thinkingEffort: 'medium' }); + }); + + test('absent entry with a partial global value keeps untouched schema defaults', () => { + // Regression for the contextSize 200K-vs-full-window bug: a global config + // that lacks contextSize must not strip the default contextSize. + const partialDefaults: IStringDictionary = { thinkingEffort: 'medium', contextSize: 200_000 }; + const resolved = resolveModelConfiguration(undefined, partialDefaults, { thinkingEffort: 'high' }); + assert.deepStrictEqual(resolved, { thinkingEffort: 'high', contextSize: 200_000 }); }); }); diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/input/chatModelConfigurationStore.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/input/chatModelConfigurationStore.test.ts index a2f2659987464..d218f429aa216 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/input/chatModelConfigurationStore.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/input/chatModelConfigurationStore.test.ts @@ -5,6 +5,7 @@ import assert from 'assert'; import { IStringDictionary } from '../../../../../../../base/common/collections.js'; +import { Emitter, Event } from '../../../../../../../base/common/event.js'; import { DisposableStore } from '../../../../../../../base/common/lifecycle.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../../base/test/common/utils.js'; import { InMemoryStorageService, StorageScope, StorageTarget } from '../../../../../../../platform/storage/common/storage.js'; @@ -22,8 +23,10 @@ const KEY = 'chat.modelConfiguration.panel'; function createStubService(global?: IStringDictionary): ILanguageModelsService { return { + onDidChangeLanguageModels: Event.None, lookupLanguageModel: (_id: string) => ({ configurationSchema: schema } as ILanguageModelChatMetadata), getModelConfiguration: (_id: string) => global, + setModelConfiguration: async (_id: string, _values: IStringDictionary) => { }, } as unknown as ILanguageModelsService; } @@ -34,6 +37,37 @@ suite('ChatModelConfigurationStore', () => { return store.add(new ChatModelConfigurationStore(() => KEY, service, storage)); } + interface IControllableService { + readonly service: ILanguageModelsService; + readonly setConfigCalls: Array<{ modelId: string; values: IStringDictionary }>; + fireModelsChanged(): void; + setRegistered(registered: boolean): void; + setGlobal(value: IStringDictionary | undefined): void; + } + + // A stub whose model registration and profile-global value can change over + // time (mirroring the asynchronous provider registration) and which records + // forwarded global writes so tests can assert no-ops are not propagated. + function createControllableService(): IControllableService { + const emitter = store.add(new Emitter()); + let registered = true; + let global: IStringDictionary | undefined; + const setConfigCalls: Array<{ modelId: string; values: IStringDictionary }> = []; + const service = { + onDidChangeLanguageModels: emitter.event, + lookupLanguageModel: (_id: string) => registered ? ({ configurationSchema: schema } as ILanguageModelChatMetadata) : undefined, + getModelConfiguration: (_id: string) => global, + setModelConfiguration: async (modelId: string, values: IStringDictionary) => { setConfigCalls.push({ modelId, values }); }, + } as unknown as ILanguageModelsService; + return { + service, + setConfigCalls, + fireModelsChanged: () => emitter.fire('copilot'), + setRegistered: (value: boolean) => { registered = value; }, + setGlobal: (value: IStringDictionary | undefined) => { global = value; }, + }; + } + test('non-default value round-trips through storage to a newly opened editor', () => { const storage = store.add(new InMemoryStorageService()); @@ -164,4 +198,50 @@ suite('ChatModelConfigurationStore', () => { // Object.prototype is untouched. assert.strictEqual(({} as IStringDictionary)['polluted'], undefined); }); + + test('re-selecting the current value does not rewrite the profile-global configuration', () => { + const storage = store.add(new InMemoryStorageService()); + const controls = createControllableService(); + const editor = createStore(storage, controls.service); + + // The first selection is a real change and is mirrored to the global. + editor.setModelConfiguration(MODEL, { thinkingEffort: 'high' }); + assert.strictEqual(controls.setConfigCalls.length, 1); + + // Re-applying the same value is a local no-op and must not touch the global. + editor.setModelConfiguration(MODEL, { thinkingEffort: 'high' }); + assert.strictEqual(controls.setConfigCalls.length, 1); + }); + + test('model change refreshes only pre-config-load snapshots, not bucket-backed ones', () => { + const storage = store.add(new InMemoryStorageService()); + // A stable, bucket-backed entry for MODEL. + storage.store(KEY, JSON.stringify({ [MODEL]: { thinkingEffort: 'high' } }), StorageScope.APPLICATION, StorageTarget.USER); + + const controls = createControllableService(); + // Nothing is registered yet, so schema defaults / global config are absent. + controls.setRegistered(false); + const editor = createStore(storage, controls.service); + + // MODEL resolves from its bucket entry even before registration (stable). + assert.deepStrictEqual(editor.getModelConfiguration(MODEL), { thinkingEffort: 'high' }); + // OTHER has no bucket entry and nothing is registered, so it caches an empty + // (poisoned) snapshot that must refresh once configuration becomes available. + const OTHER = 'copilot/other'; + assert.strictEqual(editor.getModelConfiguration(OTHER), undefined); + + const fired: string[] = []; + store.add(editor.onDidChange(id => fired.push(id))); + + // Providers register; the schema and a non-default global become available. + controls.setRegistered(true); + controls.setGlobal({ thinkingEffort: 'low' }); + controls.fireModelsChanged(); + + // Only the poisoned OTHER snapshot is dropped + refreshed; MODEL's stable + // bucket-backed snapshot survives without a duplicate refresh. + assert.deepStrictEqual(fired, [OTHER]); + assert.deepStrictEqual(editor.getModelConfiguration(OTHER), { thinkingEffort: 'low' }); + assert.deepStrictEqual(editor.getModelConfiguration(MODEL), { thinkingEffort: 'high' }); + }); });