Skip to content

Remove legacy sessionOptions string value#303002

Merged
mjbvz merged 3 commits intomicrosoft:mainfrom
mjbvz:dev/mjbvz/medical-lobster
Mar 18, 2026
Merged

Remove legacy sessionOptions string value#303002
mjbvz merged 3 commits intomicrosoft:mainfrom
mjbvz:dev/mjbvz/medical-lobster

Conversation

@mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Mar 18, 2026

No description provided.

I believe the value should always be a `ChatSessionProviderOptionItem` now. I checked the copilot chat extension too and don't see anyone using the string value directly
Copilot AI review requested due to automatic review settings March 18, 2026 21:56
@vs-code-engineering vs-code-engineering bot added this to the 1.113.0 milestone Mar 18, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Removes legacy string-typed chat session option values and standardizes option passing on ChatSessionProviderOptionItem across the proposed chat sessions provider API and parts of the workbench implementation.

Changes:

  • Updated vscode.proposed.chatSessionsProvider types so session option values are ChatSessionProviderOptionItem (not string).
  • Updated internal chat sessions service APIs and caches to store/return option items.
  • Adjusted a subset of tests/mocks to use option items instead of strings.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/vscode-dts/vscode.proposed.chatSessionsProvider.d.ts Removes string unions from session option values in the proposed API surface.
src/vs/workbench/contrib/chat/test/common/mockChatSessionsService.ts Updates mock session options storage to use option items.
src/vs/workbench/contrib/chat/test/common/chatService/chatService.test.ts Updates chat service test data to use option-item objects.
src/vs/workbench/contrib/chat/common/chatSessionsService.ts Narrows session option APIs (get/set) to option-item types.
src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts Migrates contributed session option caching and notification plumbing to option items.
src/vs/workbench/api/test/browser/mainThreadChatSessions.test.ts Partially updates tests to use option-item objects.
src/vs/workbench/api/common/extHost.protocol.ts Updates protocol DTOs for initial options + session options record to option-item objects.
src/vs/workbench/api/browser/mainThreadChatSessions.ts Narrows ObservableChatSession.options to option items.
Comments suppressed due to low confidence (1)

src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts:1185

  • notifySessionOptionsChange was narrowed to only accept IChatSessionProviderOptionItem, but there are verified core call sites still passing string values (e.g. chatInputPart.ts passes '' / modeName, newSession.ts passes value: IChatSessionProviderOptionItem | string). With the earlier removal of typeof value === 'string' ? value : value.id conversion in getSessionOptions, any string values that reach here will now be stored and later treated like an option-item object, causing type/logic errors. Please either (1) complete the migration by updating all callers + service interfaces/protocol DTOs to only use option items, or (2) keep backwards compatibility here by accepting string | IChatSessionProviderOptionItem and converting strings to { id, name } (and handling clearing via undefined/empty string) at the boundary.
	public async notifySessionOptionsChange(sessionResource: URI, updates: ReadonlyArray<{ optionId: string; value: IChatSessionProviderOptionItem }>): Promise<void> {
		if (!updates.length) {
			return;
		}
		this._logService.trace(`[ChatSessionsService] notifySessionOptionsChange: starting for ${sessionResource}, ${updates.length} update(s): [${updates.map(u => u.optionId).join(', ')}]`);
		// Fire event to notify MainThreadChatSessions (which forwards to extension host)
		// Uses fireAsync to properly await async listener work via waitUntil pattern
		await this._onRequestNotifyExtension.fireAsync({ sessionResource, updates }, CancellationToken.None);
		this._logService.trace(`[ChatSessionsService] notifySessionOptionsChange: fireAsync completed for ${sessionResource}`);
		for (const u of updates) {
			this.setSessionOption(sessionResource, u.optionId, u.value);
		}

You can also share your feedback on Copilot code review. Take the survey.

@mjbvz mjbvz enabled auto-merge March 18, 2026 22:23
[
{ optionId: 'model', value: 'claude-3.5-sonnet' },
{ optionId: 'repo', value: 'my-repo' },
{ optionId: 'model', value: { name: 'claude 3.5', id: 'claude-3.5-sonnet' } },
Copy link
Member

Choose a reason for hiding this comment

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

kinda old model?

@mjbvz mjbvz merged commit 7ddad41 into microsoft:main Mar 18, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants