diff --git a/src/vs/platform/agentHost/common/meta/agentPermissionRequestMeta.ts b/src/vs/platform/agentHost/common/meta/agentPermissionRequestMeta.ts index 0dc82fce5df0bd..fe27a9a0f17cbf 100644 --- a/src/vs/platform/agentHost/common/meta/agentPermissionRequestMeta.ts +++ b/src/vs/platform/agentHost/common/meta/agentPermissionRequestMeta.ts @@ -10,6 +10,13 @@ * A remote host describes the pending decision (run a command, read a file, …) * but does not stamp the `_meta.toolKind` rendering hint local agent adapters * provide, so the kind is recovered from here instead. + * + * Compatibility bridge, not the durable contract: `promptRequest` and + * `permissionRequest` are raw Copilot runtime payloads, not protocol fields, so + * their shape can change without a version bump and every client has to learn + * Copilot internals to render an approval. Delete this file, and its use in + * `getToolKind`, once the minimum supported host describes confirmations + * natively. */ interface IHasPermissionRequestMeta { diff --git a/src/vs/platform/agentHost/common/state/sessionReducers.ts b/src/vs/platform/agentHost/common/state/sessionReducers.ts index 129ad047429bde..64a59c3cebcb62 100644 --- a/src/vs/platform/agentHost/common/state/sessionReducers.ts +++ b/src/vs/platform/agentHost/common/state/sessionReducers.ts @@ -24,8 +24,12 @@ const PERMISSION_REQUEST_TOOL_KINDS: Readonly(CloudSandboxAgentHostContribution.ID); } + /** Test seam: overridden so a test can reach the timeout without waiting out the real budget. */ + protected get _sandboxModelWaitMs(): number { + return CopilotChatSessionsProvider.SANDBOX_MODEL_WAIT_MS; + } + /** * Commit a cloud new-session into a GitHub-managed sandbox instead of the server-run cloud * agent: provision the sandbox, then hand the session over to the remote-agent-host provider @@ -2160,7 +2167,7 @@ export class CopilotChatSessionsProvider extends Disposable implements ISessions let provisioned: ICloudSandboxProvisionedSession | undefined; // Read before provisioning: the composer session is retired below, and its selection is the // only record of what the user picked for this turn. - const selectedRawModelId = this._rawCloudModelId(session); + const selectedModel = this._selectedCloudModel(session); try { provisioned = await this._getCloudSandboxContribution().provisionSession({ repoNwo, @@ -2171,7 +2178,7 @@ export class CopilotChatSessionsProvider extends Disposable implements ISessions // Send into the session's main chat rather than `createNewChat`, which would mint an // *additional* peer chat inside a session that already has one. const chat = provisioned.session.mainChat.get(); - await this._carryModelToSandbox(provisioned, chat.resource, selectedRawModelId); + await this._carryModelToSandbox(provisioned, chat.resource, selectedModel); const committed = await provisioned.provider.sendRequest(provisioned.session.sessionId, chat.resource, options); // Retire only once the turn is dispatched; swapping earlier bounces the view home. @@ -2200,20 +2207,25 @@ export class CopilotChatSessionsProvider extends Disposable implements ISessions } /** - * The backend model id behind this composer's selection, as the sandbox knows it. + * The composer's model selection as the sandbox knows it, plus the label to name it by. * * Cloud sessions pick from the extension host's `models` option group, whose ids are the * group's own item ids, while a sandbox registers its models from what the agent host - * advertises. The two are different id spaces, so only the underlying model id crosses over. + * advertises. Different id spaces, so only the underlying model id crosses over. + * + * Only the model, because only the model exists: an option item's `modelMetadata` is hover and + * pricing detail with no configuration schema, so a cloud composer never offers a thinking + * level or context tier to carry alongside it. */ - private _rawCloudModelId(session: RemoteNewSession): string | undefined { + private _selectedCloudModel(session: RemoteNewSession): { readonly rawModelId: string; readonly label: string } | undefined { const selectedModelId = session.selectedModelId; if (!selectedModelId) { return undefined; } const { modelOption } = session.getModelOptionsSnapshot(); const item = modelOption?.group.items.find(i => i.id === selectedModelId); - return item?.modelMetadata?.id ?? item?.id ?? selectedModelId; + const rawModelId = item?.modelMetadata?.id ?? item?.id ?? selectedModelId; + return { rawModelId, label: item?.modelMetadata?.name ?? item?.name ?? rawModelId }; } /** @@ -2221,19 +2233,20 @@ export class CopilotChatSessionsProvider extends Disposable implements ISessions * * Mission Control starts no run, so this client sends that turn — and a session that has never * run has no model of its own to restore. Without this the turn carries no model at all and - * silently runs on whatever the agent host defaults to, discarding the user's pick along with - * the thinking level and context tier configured against it. + * runs on whatever the agent host defaults to. * - * A freshly connected sandbox publishes its models asynchronously, so an empty catalog here is - * "not yet" rather than "no": the model resolution is awaited while it reports `pending`, which - * is the wait {@link ISessionsProvider.getModelsSnapshot} documents. Bounded, because the turn - * cannot be held indefinitely — on timeout, or a model the sandbox genuinely does not offer, - * the host chooses, which is the behavior this had before. + * A freshly connected sandbox publishes its models asynchronously, so an empty catalog is "not + * yet" rather than "no": resolution is awaited while it reports `pending`, bounded because the + * turn cannot be held indefinitely. + * + * Every path that gives up tells the user: an absent `Message.model` means "host decides", so + * nothing downstream would report running at a capability and price they did not choose. */ - private async _carryModelToSandbox(provisioned: ICloudSandboxProvisionedSession, chatResource: URI, rawModelId: string | undefined): Promise { - if (!rawModelId) { + private async _carryModelToSandbox(provisioned: ICloudSandboxProvisionedSession, chatResource: URI, selected: { readonly rawModelId: string; readonly label: string } | undefined): Promise { + if (!selected) { return; } + const { rawModelId, label } = selected; const sessionId = provisioned.session.sessionId; const provider = provisioned.provider; @@ -2242,13 +2255,14 @@ export class CopilotChatSessionsProvider extends Disposable implements ISessions const modelTarget = provider.getModelsSnapshot(sessionId).modelTarget; if (!modelTarget) { this.logService.info(`[CopilotChatSessionsProvider] Sandbox session ${sessionId} reported no model target; letting the agent host choose.`); + this._notifySandboxModelNotApplied(label); return; } const desiredModelId = `${modelTarget}:${rawModelId}`; const store = new DisposableStore(); try { - const deadline = Date.now() + CopilotChatSessionsProvider.SANDBOX_MODEL_WAIT_MS; + const deadline = Date.now() + this._sandboxModelWaitMs; for (; ;) { const resolution = provider.getModelsSnapshot(sessionId, desiredModelId).desiredModelResolution; if (resolution.kind === 'available') { @@ -2257,6 +2271,7 @@ export class CopilotChatSessionsProvider extends Disposable implements ISessions } if (resolution.kind !== 'pending') { this.logService.info(`[CopilotChatSessionsProvider] Sandbox session ${sessionId} does not advertise model '${rawModelId}'; letting the agent host choose.`); + this._notifySandboxModelNotApplied(label); return; } const remaining = deadline - Date.now(); @@ -2267,6 +2282,7 @@ export class CopilotChatSessionsProvider extends Disposable implements ISessions : undefined; if (!published) { this.logService.warn(`[CopilotChatSessionsProvider] Sandbox session ${sessionId} had not published model '${rawModelId}' in time; letting the agent host choose.`); + this._notifySandboxModelNotApplied(label); return; } } @@ -2275,6 +2291,11 @@ export class CopilotChatSessionsProvider extends Disposable implements ISessions } } + /** Name the model so the substitution is attributable. A warning: the turn still runs. */ + private _notifySandboxModelNotApplied(label: string): void { + this.notificationService.warn(localize('sandboxModelNotApplied', "Couldn't use {0} for this session. The agent's default model will be used instead.", label)); + } + /** Retire the optimistic placeholder in favour of the session that now exists. */ private _retirePlaceholder(session: RemoteNewSession, placeholder: ISession, committed: ISession): void { this._sessionCache.delete(session.resource.toString()); diff --git a/src/vs/sessions/contrib/providers/copilotChatSessions/test/browser/copilotChatSessionsProvider.test.ts b/src/vs/sessions/contrib/providers/copilotChatSessions/test/browser/copilotChatSessionsProvider.test.ts index 34f041f2d1a271..f0d65a07847028 100644 --- a/src/vs/sessions/contrib/providers/copilotChatSessions/test/browser/copilotChatSessionsProvider.test.ts +++ b/src/vs/sessions/contrib/providers/copilotChatSessions/test/browser/copilotChatSessionsProvider.test.ts @@ -44,6 +44,7 @@ import { CloudSandboxSessionsProvider } from '../../../remoteAgentHost/browser/c import { ChatConfiguration, ChatPermissionLevel } from '../../../../../../workbench/contrib/chat/common/constants.js'; import { CopilotChatSessionsProvider, COPILOT_PROVIDER_ID, CopilotCloudSessionType, ICopilotChatSession } from '../../browser/copilotChatSessionsProvider.js'; import { ILogService, NullLogService } from '../../../../../../platform/log/common/log.js'; +import { INotificationService } from '../../../../../../platform/notification/common/notification.js'; import { ILabelService } from '../../../../../../platform/label/common/label.js'; import { IPathService } from '../../../../../../workbench/services/path/common/pathService.js'; import { MockLabelService } from '../../../../../../workbench/services/label/test/common/mockLabelService.js'; @@ -314,6 +315,9 @@ function createProviderWithConfig( onDidChangeFocusedSession: Event.None, }); instantiationService.stub(ILanguageModelsService, opts?.languageModelsService ?? { lookupLanguageModel: () => undefined }); + instantiationService.stub(INotificationService, new class extends mock() { + override warn(): void { } + }()); instantiationService.stub(ILanguageModelToolsService, { toToolReferences: () => [], }); @@ -348,19 +352,26 @@ function createProviderWithConfig( class TestSandboxCopilotProvider extends CopilotChatSessionsProvider { sandboxContribution: Pick | undefined; + /** Only the timeout test lowers this; the rest keep the real budget so they cannot race it. */ + sandboxModelWaitMs: number | undefined; + protected override _getCloudSandboxContribution(): Pick { if (!this.sandboxContribution) { throw new Error('No cloud sandbox contribution was registered'); } return this.sandboxContribution; } + + protected override get _sandboxModelWaitMs(): number { + return this.sandboxModelWaitMs ?? super._sandboxModelWaitMs; + } } function createProviderForSendTests( disposables: DisposableStore, model: MockAgentSessionsModel, sendRequest: (resource: URI, message: string, options?: IChatSendRequestOptions) => Promise, - opts?: { onDidCommitSession?: Event<{ original: URI; committed: URI }>; configurationService?: TestConfigurationService; agentHostEnabled?: boolean; getOptionGroups?: () => IChatSessionProviderOptionGroup[] | undefined }, + opts?: { onDidCommitSession?: Event<{ original: URI; committed: URI }>; configurationService?: TestConfigurationService; agentHostEnabled?: boolean; getOptionGroups?: () => IChatSessionProviderOptionGroup[] | undefined; notifications?: string[] }, ): TestSandboxCopilotProvider { const instantiationService = disposables.add(new TestInstantiationService()); @@ -406,6 +417,9 @@ function createProviderForSendTests( onDidChangeFocusedSession: Event.None, }); instantiationService.stub(ILanguageModelsService, { lookupLanguageModel: () => undefined }); + instantiationService.stub(INotificationService, new class extends mock() { + override warn(message: unknown): void { opts?.notifications?.push(String(message)); } + }()); instantiationService.stub(ILanguageModelToolsService, { toToolReferences: () => [] }); instantiationService.stub(IGitService, { openRepository: async () => undefined }); instantiationService.stub(IInstantiationService, instantiationService); @@ -1964,11 +1978,12 @@ suite('CopilotChatSessionsProvider', () => { configurationService.setUserConfiguration(RemoteAgentHostsEnabledSettingId, true); const cloudSends: string[] = []; + const notifications: string[] = []; const provider = createProviderForSendTests(disposables, model, async (_resource, message) => { cloudSends.push(message); // Never settles: these tests only assert which path the send took. return new Promise(() => { }); - }, { configurationService, getOptionGroups: opts.getOptionGroups }); + }, { configurationService, getOptionGroups: opts.getOptionGroups, notifications }); const provisionRequests: ICloudSandboxCreateSessionRequest[] = []; provider.sandboxContribution = { @@ -1980,7 +1995,7 @@ suite('CopilotChatSessionsProvider', () => { throw new Error('provisioning failed'); }, }; - return { provider, provisionRequests, cloudSends }; + return { provider, provisionRequests, cloudSends, notifications }; } /** @@ -2058,7 +2073,7 @@ suite('CopilotChatSessionsProvider', () => { // Mission Control starts no run, so a session that has never run has no model to // restore: without this the first turn would silently take the agent host default. const provisioned = provisionedSession(undefined, () => [sandboxModel('claude-sonnet-4.6')]); - const { provider } = createSandboxProvider({ + const { provider, notifications } = createSandboxProvider({ provision: async () => provisioned, getOptionGroups: () => cloudModelOptionGroup('synthetic-cloud-model', 'claude-sonnet-4.6'), }); @@ -2070,8 +2085,12 @@ suite('CopilotChatSessionsProvider', () => { await provider.sendRequest(sessionInfo.sessionId, session.mainChat.get().resource, { query: 'fix it' }); // The id crosses id spaces by backend model id, and arrives as carried over: the user - // picked it for the composer, not for the session that replaced it. - assert.deepStrictEqual(provisioned.modelSelections, [{ modelId: 'agent-host-copilot:claude-sonnet-4.6', source: ChatModelSource.CarriedOver }]); + // picked it for the composer, not for the session that replaced it. Applying the pick + // is the silent case — nothing to tell the user about. + assert.deepStrictEqual( + { selections: provisioned.modelSelections, notifications }, + { selections: [{ modelId: 'agent-host-copilot:claude-sonnet-4.6', source: ChatModelSource.CarriedOver }], notifications: [] } + ); }); test('waits for a sandbox catalog that is still arriving rather than sending without the model', async () => { @@ -2102,11 +2121,12 @@ suite('CopilotChatSessionsProvider', () => { ); }); - test('leaves the model to the agent host when the sandbox does not advertise it', async () => { - // Sending an unroutable id would fail the turn outright, so an unmatched pick keeps - // the previous behavior of letting the host choose. + test('tells the user when the sandbox does not advertise the model they picked', async () => { + // Sending an unroutable id would fail the turn outright, so an unmatched pick still + // lets the host choose — but an absent `Message.model` means "host decides", so + // nothing else would report the substitution. const provisioned = provisionedSession(undefined, () => [sandboxModel('gpt-5')]); - const { provider } = createSandboxProvider({ + const { provider, notifications } = createSandboxProvider({ provision: async () => provisioned, getOptionGroups: () => cloudModelOptionGroup('synthetic-cloud-model', 'claude-sonnet-4.6'), }); @@ -2117,7 +2137,32 @@ suite('CopilotChatSessionsProvider', () => { await provider.sendRequest(sessionInfo.sessionId, session.mainChat.get().resource, { query: 'fix it' }); - assert.deepStrictEqual(provisioned.modelSelections, []); + assert.deepStrictEqual( + { selections: provisioned.modelSelections, notified: notifications.length, namesModel: notifications[0]?.includes('claude-sonnet-4.6') }, + { selections: [], notified: 1, namesModel: true } + ); + }); + + test('tells the user when the catalog never arrives before the turn is dispatched', async () => { + // The likeliest fallback in practice is a slow sandbox rather than a missing model, so + // the timeout has to be as visible as a conclusive miss. + const provisioned = provisionedSession(undefined, () => []); + const { provider, notifications } = createSandboxProvider({ + provision: async () => provisioned, + getOptionGroups: () => cloudModelOptionGroup('synthetic-cloud-model', 'claude-sonnet-4.6'), + }); + provider.sandboxModelWaitMs = 1; + const sessionInfo = provider.createNewSession(repoWorkspace, CopilotCloudSessionType.id); + const session = provider.getSession(sessionInfo.sessionId)!; + session.setUseSandbox(true); + provider.setModel(sessionInfo.sessionId, session.mainChat.get().resource, 'synthetic-cloud-model', ChatModelSource.Chosen); + + await provider.sendRequest(sessionInfo.sessionId, session.mainChat.get().resource, { query: 'fix it' }); + + assert.deepStrictEqual( + { selections: provisioned.modelSelections, notified: notifications.length, namesModel: notifications[0]?.includes('claude-sonnet-4.6') }, + { selections: [], notified: 1, namesModel: true } + ); }); test('provisions a sandbox and replaces the draft with the committed session', async () => { diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostLanguageModelProvider.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostLanguageModelProvider.ts index 455c2e36dbdde9..984ef4715e6204 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostLanguageModelProvider.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostLanguageModelProvider.ts @@ -7,7 +7,6 @@ import { CancellationToken } from '../../../../../../base/common/cancellation.js import { Codicon } from '../../../../../../base/common/codicons.js'; import { Emitter } from '../../../../../../base/common/event.js'; import { Disposable } from '../../../../../../base/common/lifecycle.js'; -import { formatTokenCount } from '../../../../../../base/common/numbers.js'; import { localize } from '../../../../../../nls.js'; import { readAgentModelNoticesMeta } from '../../../../../../platform/agentHost/common/agentModelNotices.js'; import { ConfigSchema, SessionModelInfo } from '../../../../../../platform/agentHost/common/state/sessionState.js'; @@ -18,20 +17,6 @@ import { getReasoningEffortDescription, getReasoningEffortLabel } from '../../.. import { nullExtensionDescription } from '../../../../../services/extensions/common/extensions.js'; import { AUTO_RAW_MODEL_ID, COPILOT_VENDOR_ID, ILanguageModelChatMetadata, ILanguageModelChatMetadataAndIdentifier, ILanguageModelChatProvider, ILanguageModelConfigurationSchema, ILanguageModelsService } from '../../../common/languageModels.js'; -/** - * Config key naming the context-window tier a host accepts on a model selection. Its values are - * tier names (`default` / `long_context`), because a host driving the Copilot SDK has no per-model - * token counts to offer. - */ -const CONTEXT_TIER_CONFIG_KEY = 'contextTier'; - -/** - * Config key naming the numeric context-window picker the workbench's own Copilot catalogue - * synthesizes from CAPI billing. Read here only as the source of the token counts used to label - * {@link CONTEXT_TIER_CONFIG_KEY}; it is never surfaced to a host, which would not understand it. - */ -const CONTEXT_SIZE_CONFIG_KEY = 'contextSize'; - /** * Returns whether an agent host provider exposes a synthetic "Auto" model to * fall back to. @@ -171,7 +156,7 @@ export class AgentHostLanguageModelProvider extends Disposable implements ILangu toolCalling: true, agentMode: true, }, - configurationSchema: this._toLanguageModelConfigurationSchema(m.configSchema, known), + configurationSchema: this._toLanguageModelConfigurationSchema(m.configSchema), }, }; }); @@ -180,14 +165,18 @@ export class AgentHostLanguageModelProvider extends Disposable implements ILangu /** * The workbench catalogue entry describing the same model, when one is known. * - * A host that derives its model list from the Copilot SDK advertises only what the SDK gave it: - * no billing, and no per-tier context windows. The workbench already holds that detail for the - * same models — the Copilot vendor's catalogue is CAPI-backed — so the two are matched by model - * id and the host's list is enriched from it, which is how the GitHub desktop app renders real - * token counts for a sandbox session. + * A host driving the Copilot SDK advertises no billing and, where the SDK omits them, no token + * limits — so the CAPI-backed Copilot catalogue fills the gaps. * - * Restricted to models billed through Copilot (a `copilot` picker group), so a model reached - * over a direct third-party transport is never labelled with Copilot's prices. + * Matching by raw model id does not prove the two describe the same offering — they can differ + * by entitlement, policy, rollout or billing route — so this is restricted to models billed + * through Copilot and only ever fills fields the host left absent. + * + * The token-limit half is a shim: those are native protocol fields, and a host that populates + * them makes the fallback dead. The billing half is not. Pricing is deliberately kept off the + * agent host protocol as operator-sensitive, so a client that wants to show it has to source it + * itself. Removing this join therefore means dropping pricing for these models, which is a + * product decision rather than a cleanup. */ private _catalogueEntryFor(model: SessionModelInfo, group: ILanguageModelChatMetadata['modelGroup']): ILanguageModelChatMetadata | undefined { if (!this._catalogue || group?.id !== COPILOT_VENDOR_ID) { @@ -203,45 +192,17 @@ export class AgentHostLanguageModelProvider extends Disposable implements ILangu } /** - * The distinct context-window sizes a catalogue entry offers, ascending, or `undefined` when it - * offers no real choice. Sourced from the numeric `contextSize` picker the Copilot catalogue - * synthesizes from CAPI billing, which is the only place these token counts exist. - */ - private static _contextWindowTiers(metadata: ILanguageModelChatMetadata | undefined): number[] | undefined { - const values = metadata?.configurationSchema?.properties?.[CONTEXT_SIZE_CONFIG_KEY]?.enum; - if (!values?.length) { - return undefined; - } - const sizes = [...new Set(values.filter((value): value is number => typeof value === 'number'))].sort((a, b) => a - b); - return sizes.length > 1 ? sizes : undefined; - } - - /** - * Labels for a host's `contextTier` enum, as token counts rather than tier names. + * Translate a host's model {@link ConfigSchema} into the picker's schema shape. * - * The host names the tiers (`default` / `long_context`) because the SDK exposes no per-model - * windows, but the picker is far more useful showing "264K" / "1M" — what the GitHub desktop - * app displays for the same session. The wire value stays the tier name the host accepts; only - * the label changes. + * Values and display text belong to the producer; the workbench only picks the group + * ({@link _groupForConfigKey}). The one exception is a reasoning-effort enum with no display + * text at all, labelled locally rather than rendering raw values like `xhigh`. * - * Returns `undefined` when the catalogue offers no distinct long-context tier (or does not know - * the model), which drops the property and hides the picker rather than offering a choice that - * has no effect — matching how the desktop app suppresses it. + * A property is never dropped for want of local enrichment: a host advertises one because it + * will honour it, and a new model, a staged rollout and an unresolved catalogue all look + * identical from here. */ - private static _contextTierLabels(values: readonly unknown[] | undefined, known: ILanguageModelChatMetadata | undefined): string[] | undefined { - const tiers = AgentHostLanguageModelProvider._contextWindowTiers(known); - if (!tiers || !values?.length) { - return undefined; - } - // The host orders its tiers from smallest window to largest, so they align with the sorted - // sizes by position. A tier list of a different length is not one this mapping understands. - if (values.length !== tiers.length || !values.every(value => typeof value === 'string')) { - return undefined; - } - return tiers.map(formatTokenCount); - } - - private _toLanguageModelConfigurationSchema(schema: ConfigSchema | undefined, known?: ILanguageModelChatMetadata): ILanguageModelConfigurationSchema | undefined { + private _toLanguageModelConfigurationSchema(schema: ConfigSchema | undefined): ILanguageModelConfigurationSchema | undefined { if (!schema) { return undefined; } @@ -255,25 +216,13 @@ export class AgentHostLanguageModelProvider extends Disposable implements ILangu ? AgentHostLanguageModelProvider._reasoningEffortDisplay(key, property.enum) : undefined; - let enumItemLabels = property.enumLabels ?? effortDisplay?.labels; - if (key === CONTEXT_TIER_CONFIG_KEY) { - const tierLabels = AgentHostLanguageModelProvider._contextTierLabels(property.enum, known); - if (!tierLabels) { - // No real choice to offer (or no catalogue entry to size it with): drop the - // property so the picker hides rather than showing tier names that read as a - // setting the user cannot evaluate. - continue; - } - enumItemLabels = tierLabels; - } - properties[key] = { type: property.type, title: property.title, description: property.description, default: property.default, enum: property.enum, - enumItemLabels, + enumItemLabels: property.enumLabels ?? effortDisplay?.labels, enumDescriptions: property.enumDescriptions ?? effortDisplay?.descriptions, readOnly: property.readOnly, group: AgentHostLanguageModelProvider._groupForConfigKey(key), @@ -332,7 +281,7 @@ export class AgentHostLanguageModelProvider extends Disposable implements ILangu // choose a thinking level or a context window. case 'reasoningEffort': return 'navigation'; case 'contextSize': - case CONTEXT_TIER_CONFIG_KEY: return 'tokens'; + case 'contextTier': return 'tokens'; default: return undefined; } } diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts index 44d02bf689e634..d413de68f5e421 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts @@ -1572,6 +1572,10 @@ function buildTerminalToolSpecificData( ...existing, kind: 'terminal', commandLine, + // Read-only for the same reason as a generic confirmation input: this + // adapter never returns an edited command to the host, so an editable + // field would collect a change and then run what the agent proposed. + editable: false, intention: tc.intention ?? existing?.intention, language: existing?.language ?? getTerminalLanguage(tc), autoApproveRuleResolvable: readToolCallMeta(tc).autoApproveRuleResolvable ?? existing?.autoApproveRuleResolvable, @@ -2354,7 +2358,11 @@ export function toolCallStateToInvocation(tc: ToolCallState, subAgentInvocationI if (toolInput) { let rawInput: unknown; try { rawInput = JSON.parse(toolInput); } catch { rawInput = { input: toolInput }; } - toolSpecificData = { kind: 'input', rawInput }; + // Read-only regardless of `tc.editable`: approving with an edited input means + // sending it back as `chat/toolCallConfirmed.editedToolInput`, which this adapter + // does not do, so an editable field would collect a change and then run the + // command the agent originally proposed. + toolSpecificData = { kind: 'input', rawInput, editable: false }; } } diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolConfirmationSubPart.ts b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolConfirmationSubPart.ts index d2dc4608ea64ad..3f4b5d2ca3685d 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolConfirmationSubPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolConfirmationSubPart.ts @@ -107,8 +107,9 @@ export class ChatTerminalToolConfirmationSubPart extends BaseChatToolInvocationS const initialContent = terminalData.presentationOverrides?.commandLine ?? terminalData.confirmation?.commandLine ?? (terminalData.commandLine.toolEdited ?? terminalData.commandLine.original).trimStart(); const cdPrefix = terminalData.confirmation?.cdPrefix ?? ''; // When presentationOverrides is set, the editor should be read-only since the displayed content - // differs from the actual command (e.g., extracted Python code vs full python -c command) - const isReadOnly = !!terminalData.presentationOverrides; + // differs from the actual command (e.g., extracted Python code vs full python -c command). + // A producer that cannot apply an edited command opts out the same way. + const isReadOnly = !!terminalData.presentationOverrides || terminalData.editable === false; const autoApproveEnabled = this.configurationService.getValue(TerminalContribSettingId.EnableAutoApprove) === true; // Custom actions typically come pre-computed from the run in terminal tool, but they can @@ -176,16 +177,18 @@ export class ChatTerminalToolConfirmationSubPart extends BaseChatToolInvocationS uri: model.uri, chatSessionResource: this.context.element.sessionResource }); - this._register(model.onDidChangeContent(() => { - const currentValue = model.getValue(); - // Only set userEdited if the content actually differs from the initial value - // Prepend cd prefix back if it was extracted for display - if (currentValue !== initialContent) { - terminalData.commandLine.userEdited = cdPrefix + currentValue; - } else { - terminalData.commandLine.userEdited = undefined; - } - })); + if (!isReadOnly) { + this._register(model.onDidChangeContent(() => { + const currentValue = model.getValue(); + // Only set userEdited if the content actually differs from the initial value + // Prepend cd prefix back if it was extracted for display + if (currentValue !== initialContent) { + terminalData.commandLine.userEdited = cdPrefix + currentValue; + } else { + terminalData.commandLine.userEdited = undefined; + } + })); + } const elements = h('.chat-confirmation-message-terminal', [ h('.chat-confirmation-message-terminal-editor@editor'), h('.chat-confirmation-message-terminal-disclaimer@disclaimer'), diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationSubPart.ts b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationSubPart.ts index 9df90f3857df74..5fb46fc58290f5 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationSubPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationSubPart.ts @@ -189,6 +189,8 @@ export class ToolConfirmationSubPart extends AbstractToolConfirmationSubPart { elements.editor.appendChild(titleEl); const inputData = toolInvocation.toolSpecificData; + // Editable unless a producer opts out, so tools that predate the flag are unchanged. + const isEditable = inputData.editable ?? true; const codeBlockRenderOptions: ICodeBlockRenderOptions = { hideToolbar: true, @@ -197,7 +199,7 @@ export class ToolConfirmationSubPart extends AbstractToolConfirmationSubPart { verticalPadding: 5, editorOptions: { wordWrap: 'off', - readOnly: false, + readOnly: !isEditable, ariaLabel: this.getTitle(), } }; @@ -265,13 +267,15 @@ export class ToolConfirmationSubPart extends AbstractToolConfirmationSubPart { uri: model.uri, chatSessionResource: this.context.element.sessionResource }); - this._register(model.onDidChangeContent(e => { - try { - inputData.rawInput = JSON.parse(model.getValue()); - } catch { - // ignore - } - })); + if (isEditable) { + this._register(model.onDidChangeContent(e => { + try { + inputData.rawInput = JSON.parse(model.getValue()); + } catch { + // ignore + } + })); + } elements.editor.append(editor.object.element); diff --git a/src/vs/workbench/contrib/chat/common/chatService/chatService.ts b/src/vs/workbench/contrib/chat/common/chatService/chatService.ts index 1ad2106bc39e8a..e01b92b77e5632 100644 --- a/src/vs/workbench/contrib/chat/common/chatService/chatService.ts +++ b/src/vs/workbench/contrib/chat/common/chatService/chatService.ts @@ -659,6 +659,17 @@ export interface IChatTerminalToolInvocationData { // isSandboxWrapped boolean to run in the terminal (potentially different from original command) isSandboxWrapped?: boolean; }; + /** + * Whether the user may edit the command before confirming. + * + * Omitted means editable, the historical behavior for the built-in terminal + * tool, which runs `commandLine.userEdited` when it is set. A producer whose + * confirmation does not return the edit — an agent-host session, whose edit + * would have to travel back as `chat/toolCallConfirmed.editedToolInput` — + * MUST set `false`. Letting someone edit a command they are approving and + * then running the original is worse than showing it read-only. + */ + editable?: boolean; /** * LM-generated intention describing why the command is being run, shown * above the command in the terminal tool card. Set by the Agent Host; the @@ -802,6 +813,21 @@ export interface IChatToolInputInvocationData { rawInput: any; /** Optional MCP App UI metadata for rendering during and after tool execution */ mcpAppData?: ChatMcpAppData; + /** + * Whether the user may edit {@link rawInput} before confirming. + * + * Omitted means editable, the historical behavior: the confirmation editor + * writes back into `rawInput`, and for an extension-contributed tool + * `ILanguageModelToolsService` then invokes it with that value as its + * parameters. That path always honours an edit, which is why no opt-out + * existed before. + * + * A producer whose confirmation does not run through it — an agent-host + * session, whose edit would have to travel back as + * `chat/toolCallConfirmed.editedToolInput` — MUST set `false`. Inviting an + * edit and then running the original is worse than showing none. + */ + editable?: boolean; } export const enum ToolConfirmKind { diff --git a/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostLanguageModelProvider.test.ts b/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostLanguageModelProvider.test.ts index 268195bf67e644..c33fbaa8886fb7 100644 --- a/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostLanguageModelProvider.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostLanguageModelProvider.test.ts @@ -76,9 +76,7 @@ suite('AgentHostLanguageModelProvider', () => { const infos = await provider.provideLanguageModelChatInfo(undefined, CancellationToken.None); assert.deepStrictEqual( Object.fromEntries(Object.entries(infos[0].metadata.configurationSchema?.properties ?? {}).map(([key, property]) => [key, property.group])), - // `contextTier` needs token counts from the catalogue to be worth showing, and there is - // none here; see the context-tier tests below. - { reasoningEffort: 'navigation' } + { reasoningEffort: 'navigation', contextTier: 'tokens' } ); }); @@ -269,9 +267,9 @@ suite('AgentHostLanguageModelProvider', () => { }; } - test('labels the host context tiers with the token counts from the workbench catalogue', async () => { - // The host names its tiers because the SDK gives it no per-model windows. The workbench - // already knows them, so the picker shows the numbers while the wire value stays the tier. + test('publishes the host context tiers with the host labels, not catalogue token counts', async () => { + // The host's tier enum and the catalogue's `contextSize` list are independent catalogues, + // so pairing them by position mislabels the moment either changes. The host names its own. const { catalogue: known } = catalogue([{ id: 'claude-opus-5', contextSizes: [264_000, 1_000_000] }]); const provider = store.add(new AgentHostLanguageModelProvider('agent-host-copilot', 'copilot', known)); provider.updateModels([ @@ -289,13 +287,13 @@ suite('AgentHostLanguageModelProvider', () => { const tier = infos[0].metadata.configurationSchema?.properties?.contextTier; assert.deepStrictEqual( { enum: tier?.enum, labels: tier?.enumItemLabels, group: tier?.group }, - { enum: ['default', 'long_context'], labels: ['264K', '1M'], group: 'tokens' } + { enum: ['default', 'long_context'], labels: ['Default', 'Long context'], group: 'tokens' } ); }); - test('drops the context tier when there is no distinct long-context window to choose', async () => { - // Matches how the GitHub desktop app suppresses the picker: an unknown model, or one whose - // tiers are the same size, offers a choice the user cannot act on. + test('keeps a host config property the catalogue cannot enrich', async () => { + // A host advertises a property because it will honour it, and an unknown model, a staged + // rollout and an unresolved catalogue are indistinguishable from here. const { catalogue: known } = catalogue([{ id: 'known-single-tier', contextSizes: [200_000] }]); const provider = store.add(new AgentHostLanguageModelProvider('agent-host-copilot', 'copilot', known)); const contextTierOnly = { @@ -311,8 +309,8 @@ suite('AgentHostLanguageModelProvider', () => { assert.deepStrictEqual( infos.map(info => ({ id: info.metadata.id, properties: Object.keys(info.metadata.configurationSchema?.properties ?? {}) })), [ - { id: 'known-single-tier', properties: [] }, - { id: 'unknown-to-catalogue', properties: [] }, + { id: 'known-single-tier', properties: ['contextTier'] }, + { id: 'unknown-to-catalogue', properties: ['contextTier'] }, ] ); }); diff --git a/src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts b/src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts index 07a0bb98aed8bb..e191bf2d8ec5e9 100644 --- a/src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts @@ -18,7 +18,7 @@ import { createAgentHostResourceUriMapper, fromAgentHostUri, toAgentHostContentU import { buildSubagentChatUri, ChatInputAnswerState, ChatInputAnswerValueKind, ChatInputQuestionKind, ChatInputResponseKind, createErrorResponsePart, MessageAttachmentKind, MessageKind, ToolCallContributorKind, ToolCallRiskAssessmentKind, ToolCallRiskAssessmentStatus, ToolCallStatus, ToolCallConfirmationReason, ToolResultContentType, TurnState, ResponsePartKind, readUsageInfoMeta, withMessageHiddenFromTranscript, withMessageRequestHiddenFromTranscript, type ActiveTurn, type ICompletedToolCall, type ToolCallPendingConfirmationState, type ToolCallRunningState, type Turn, type ToolCallResponsePart, ToolCallCancellationReason, type Message, type ToolResultContent } from '../../../../../../platform/agentHost/common/state/sessionState.js'; import { ChatTranscriptContextAttachmentDisplayKind, IChatRequestTranscriptContextVariableEntry, toChatTranscriptContextAttachmentMeta } from '../../../common/attachments/chatVariableEntries.js'; import { ChatRequestOriginKind } from '../../../common/chatRequestOrigin.js'; -import { IChatToolInvocation, IChatToolInvocationSerialized, ToolConfirmKind, type IChatMarkdownContent, type IChatTerminalToolInvocationData, type IChatThinkingPart, type IChatUsage } from '../../../common/chatService/chatService.js'; +import { IChatToolInvocation, IChatToolInvocationSerialized, ToolConfirmKind, type IChatMarkdownContent, type IChatTerminalToolInvocationData, type IChatThinkingPart, type IChatToolInputInvocationData, type IChatUsage } from '../../../common/chatService/chatService.js'; import { isToolResultInputOutputDetails, type IToolResultInputOutputDetails, ToolDataSource, ToolInvocationPresentation } from '../../../common/tools/languageModelToolsService.js'; import { turnsToHistory as rawTurnsToHistory, activeTurnToProgress as rawActiveTurnToProgress, completedToolCallToSerialized, containsAutomaticReplyAnswer, createInputRequestCarousel, messageAttachmentsToVariableData, shouldObserveSubagentChat, toolCallStateToInvocation as rawToolCallStateToInvocation, toolCallStateToPreparedInvocation as rawToolCallStateToPreparedInvocation, toolCallStateToStreamingInvocation, finalizeToolInvocation as rawFinalizeToolInvocation, updateRunningToolSpecificData as rawUpdateRunningToolSpecificData, updateStreamingToolInvocation, usageInfoToAutoModeResolution, usageInfoToChatUsage, usageInfoToQuotas, formatTurnResponseDetails, rewriteAgentHostLinkTarget, rewriteMarkdownLinks, type TurnModelLookup } from '../../../browser/agentSessions/agentHost/stateToProgressAdapter.js'; import { getQuotaReset } from '../../../../../services/chat/common/chatEntitlementService.js'; @@ -1594,10 +1594,14 @@ suite('stateToProgressAdapter', () => { kind: invocation.toolSpecificData?.kind, command: (invocation.toolSpecificData as IChatTerminalToolInvocationData | undefined)?.commandLine.original, language: (invocation.toolSpecificData as IChatTerminalToolInvocationData | undefined)?.language, + // Read-only for the same reason as a generic input: the edit is never returned, so + // an editable command line would run the one the agent proposed. + editable: (invocation.toolSpecificData as IChatTerminalToolInvocationData | undefined)?.editable, }, { kind: 'terminal', command: 'rg -n "sandbox" --glob "*.ts"', language: 'shellscript', + editable: false, }); }); @@ -1624,6 +1628,31 @@ suite('stateToProgressAdapter', () => { }); }); + test('presents a generic confirmation input read-only, since edits are never sent back', () => { + // The confirmation editor writes into `rawInput`, but this adapter never returns it as + // `editedToolInput` — an editable field would run the command the agent proposed. + const tc: ToolCallPendingConfirmationState = { + toolCallId: 'tc-perm-path', + toolName: 'shell', + displayName: 'Shell', + invocationMessage: 'Access paths', + status: ToolCallStatus.PendingConfirmation, + toolInput: '/a/one.ts, /a/two.ts', + // A host claiming otherwise does not change that: the round trip is what is missing. + editable: true, + _meta: { requestId: 'req-3', promptRequest: { kind: 'path', accessKind: 'shell' } }, + }; + + const invocation = toolCallStateToInvocation(tc); + assert.deepStrictEqual({ + kind: invocation.toolSpecificData?.kind, + editable: (invocation.toolSpecificData as IChatToolInputInvocationData | undefined)?.editable, + }, { + kind: 'input', + editable: false, + }); + }); + test('does not render a path permission as a terminal command', () => { // A path request's subject is a list of paths, not a command line, // even when its `accessKind` is `shell`. @@ -3322,8 +3351,9 @@ suite('stateToProgressAdapter', () => { contributor: { kind: ToolCallContributorKind.MCP, customizationId: 'docs-customization' }, _meta: meta, }); - // Confirmation state carries the raw input but does not mount the App. - assert.deepStrictEqual(invocation.toolSpecificData, { kind: 'input', rawInput: { topic: 'metadata' } }); + // Confirmation state carries the raw input but does not mount the App. It is read-only: + // this adapter never returns an edited input to the host. + assert.deepStrictEqual(invocation.toolSpecificData, { kind: 'input', rawInput: { topic: 'metadata' }, editable: false }); let stateChanged = false; const disposable = autorun(r => {