From 137a1e18e0081e58969bf7d2b7185849b14acabf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joaqu=C3=ADn=20Ruales?= <1588988+jruales@users.noreply.github.com> Date: Sat, 19 Sep 2026 15:10:52 -0700 Subject: [PATCH 1/4] Fix pending Copilot steering message edits Fixes #325884 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../agentHost/node/copilot/copilotAgent.ts | 5 +- .../node/copilot/copilotAgentSession.ts | 150 +++++--- .../test/node/copilotAgentSession.test.ts | 324 ++++++++++++++++-- 3 files changed, 416 insertions(+), 63 deletions(-) diff --git a/src/vs/platform/agentHost/node/copilot/copilotAgent.ts b/src/vs/platform/agentHost/node/copilot/copilotAgent.ts index cf1879240cfff4..a9cf050ed5308a 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotAgent.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotAgent.ts @@ -4341,10 +4341,7 @@ export class CopilotAgent extends Disposable implements IAgent { return; } - // Steering: send with mode 'immediate' so the SDK injects it mid-turn - if (steeringMessage) { - target.sendSteering(steeringMessage); - } + void target.setPendingSteering(steeringMessage); // Queued messages are consumed by the server (AgentSideEffects) // which dispatches ChatTurnStarted and calls sendMessage directly. diff --git a/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts b/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts index 03f3625620f9eb..2813021ac1d35d 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts @@ -1085,7 +1085,9 @@ export class CopilotAgentSession extends Disposable { private readonly _sandboxConfigSequencer = new Sequencer(); private readonly _mcpEnablementSequencer = new Sequencer(); private readonly _mcpServerLifecycleSequencer = new SequencerByKey(); - private readonly _steeringMessagesInFlight = new Set(); + private readonly _messageQueueSequencer = new Sequencer(); + private _desiredPendingSteering: PendingMessage | undefined; + private _submittedPendingSteering: { pendingMessage: PendingMessage; sdkMessageId?: string } | undefined; /** * Steering messages that have been accepted by the SDK but not yet * surfaced to the chat UI as a separate user message. When the SDK @@ -1454,17 +1456,14 @@ export class CopilotAgentSession extends Disposable { return newTurnId; } - /** - * Drains any steering messages we acknowledged to the SDK but never - * promoted to their own turn (e.g. on abort or session dispose). Fires - * `steering_consumed` so the chat UI removes the lingering pending - * steering bubble even when no fresh `user.message` arrives. - */ + /** Clears pending steering on abort or disposal, including edits still being prepared. */ private _drainPendingSteeringFlips(): void { - if (this._pendingSteeringFlips.size === 0) { - return; + const ids = new Set(this._pendingSteeringFlips.keys()); + if (this._desiredPendingSteering) { + ids.add(this._desiredPendingSteering.id); } - const ids = [...this._pendingSteeringFlips.keys()]; + this._desiredPendingSteering = undefined; + this._submittedPendingSteering = undefined; this._pendingSteeringFlips.clear(); for (const id of ids) { this._onDidSessionProgress.fire({ @@ -1492,6 +1491,7 @@ export class CopilotAgentSession extends Disposable { for (const [id, msg] of this._pendingSteeringFlips) { if (msg.message.text === content) { this._pendingSteeringFlips.delete(id); + this._didConsumePendingSteering(msg); return msg; } if (msg.message.text.length > 0 @@ -1502,11 +1502,23 @@ export class CopilotAgentSession extends Disposable { } if (substringMatch) { this._pendingSteeringFlips.delete(substringMatch[0]); + this._didConsumePendingSteering(substringMatch[1]); return substringMatch[1]; } return undefined; } + private _didConsumePendingSteering(steeringMessage: PendingMessage): void { + if (!equals(this._submittedPendingSteering?.pendingMessage, steeringMessage)) { + return; + } + this._submittedPendingSteering = undefined; + if (equals(this._desiredPendingSteering, steeringMessage)) { + this._desiredPendingSteering = undefined; + } + void this._syncPendingSteering(); + } + private _parentToolCallIdForSubagentEvent(e: { readonly agentId?: string }): string | undefined { return e.agentId ? this._parentToolCallIdsByAgentId.get(e.agentId) : undefined; } @@ -3001,10 +3013,10 @@ export class CopilotAgentSession extends Disposable { await this.applyMode(mode); let result: CopilotCommandInvocationResult; try { - result = await this._wrapper.session.rpc.commands.invoke({ + result = await this._messageQueueSequencer.queue(() => this._wrapper.session.rpc.commands.invoke({ name: runtimeSlashCommand.name, ...(slashCommand.rawRest.length > 0 ? { input: slashCommand.rawRest } : {}), - }); + })); } catch (err) { this._logService.error(err, `[Copilot:${this.sessionId}] rpc.commands.invoke(${slashCommand.command}) failed`); throw err; @@ -3058,7 +3070,7 @@ export class CopilotAgentSession extends Disposable { const sendingTurn = this._currentTurn.value; sendingTurn?.markProviderCallPending(); try { - await this._otelService.withTraceContext(traceContext, () => { + await this._messageQueueSequencer.queue(() => this._otelService.withTraceContext(traceContext, async () => { if (!this._environmentService.isBuilt && prompt === '$error') { return this._wrapper.session.rpc.sendMessages({ messages: [{ prompt }], @@ -3066,7 +3078,7 @@ export class CopilotAgentSession extends Disposable { }); } return this._wrapper.session.send({ prompt, attachments: sdkAttachments?.length ? sdkAttachments : undefined }); - }); + })); sendingTurn?.markProviderCallResolved(); } catch (error) { sendingTurn?.markProviderCallRejected(); @@ -3434,37 +3446,96 @@ export class CopilotAgentSession extends Disposable { return this._configurationService.getRootValue(platformRootSchema, AgentHostAutoReplyEnabledConfigKey) === true; } - async sendSteering(steeringMessage: PendingMessage): Promise { - if (this._steeringMessagesInFlight.has(steeringMessage.id) || this._pendingSteeringFlips.has(steeringMessage.id)) { - return; - } - this._steeringMessagesInFlight.add(steeringMessage.id); - this._logService.info(`[Copilot:${this.sessionId}] Sending steering message: "${steeringMessage.message.text.substring(0, 100)}"`); - try { + /** Synchronizes the single protocol steering message with the SDK's pending steering queue. */ + setPendingSteering(steeringMessage: PendingMessage | undefined): Promise { + this._desiredPendingSteering = steeringMessage; + return this._syncPendingSteering(); + } + + private _syncPendingSteering(): Promise { + const token = this._abortToken; + return this._messageQueueSequencer.queue(() => this._reconcilePendingSteering(token)).catch(err => { + this._logService.error(`[Copilot:${this.sessionId}] Failed to synchronize pending steering`, err); + }); + } + + private async _reconcilePendingSteering(token: CancellationToken): Promise { + while (!token.isCancellationRequested && !this.isDisposed && !equals(this._submittedPendingSteering?.pendingMessage, this._desiredPendingSteering)) { + const submitted = this._submittedPendingSteering; + if (submitted) { + if (!await this._removePendingSteering(submitted, token)) { + return; + } + this._pendingSteeringFlips.delete(submitted.pendingMessage.id); + if (this._submittedPendingSteering === submitted) { + this._submittedPendingSteering = undefined; + } + } + + const desired = this._desiredPendingSteering; + if (!desired) { + return; + } + await this._reconcileMcpServerEnablement(); - this._pendingSteeringFlips.set(steeringMessage.id, steeringMessage); - const sdkAttachments = await this._toSdkAttachments(steeringMessage.message.attachments); + if (token.isCancellationRequested || this.isDisposed || !equals(this._desiredPendingSteering, desired)) { + continue; + } + const sdkAttachments = await this._toSdkAttachments(desired.message.attachments); + if (token.isCancellationRequested || this.isDisposed || !equals(this._desiredPendingSteering, desired)) { + continue; + } // Steering is injected into the active turn and never fires the SDK's `user-prompt-submitted` // hook, so the read-only snapshot signal can't ride `additionalContext` here. Fold it into the // prompt as a `` block instead: the runtime forwards it to the model, and the host's // `stripPromptScaffolding` removes it from the displayed message (#331154). - const snapshotReminder = this._snapshotReadonlyReminder(steeringMessage.message.attachments); + const snapshotReminder = this._snapshotReadonlyReminder(desired.message.attachments); const steeringPrompt = snapshotReminder - ? `${steeringMessage.message.text}\n\n\n${snapshotReminder}\n` - : steeringMessage.message.text; - await this._wrapper.session.send({ - prompt: steeringPrompt, - attachments: sdkAttachments?.length ? sdkAttachments : undefined, - mode: 'immediate', - }); - } catch (err) { - this._pendingSteeringFlips.delete(steeringMessage.id); - this._logService.error(`[Copilot:${this.sessionId}] Steering message failed`, err); - } finally { - this._steeringMessagesInFlight.delete(steeringMessage.id); + ? `${desired.message.text}\n\n\n${snapshotReminder}\n` + : desired.message.text; + const sending: NonNullable = { pendingMessage: desired }; + this._submittedPendingSteering = sending; + this._pendingSteeringFlips.set(desired.id, desired); + try { + sending.sdkMessageId = await this._wrapper.session.send({ + prompt: steeringPrompt, + displayPrompt: desired.message.text, + attachments: sdkAttachments?.length ? sdkAttachments : undefined, + mode: 'immediate', + }); + } catch (err) { + this._pendingSteeringFlips.delete(desired.id); + if (this._submittedPendingSteering === sending) { + this._submittedPendingSteering = undefined; + } + throw err; + } } } + private async _removePendingSteering(submitted: NonNullable, token: CancellationToken): Promise { + const pending = await this._wrapper.session.rpc.queue.pendingItems(); + if (token.isCancellationRequested || this.isDisposed) { + return false; + } + if (this._submittedPendingSteering !== submitted) { + return true; + } + const queued = pending.items.find(item => submitted.sdkMessageId !== undefined && item.messageId === submitted.sdkMessageId); + if (queued) { + return (await this._wrapper.session.rpc.queue.removeAt({ id: queued.id })).removed; + } + const steering = pending.steeringMessages.slice(pending.inFlightSteeringCount ?? 0); + if (steering.length === 0) { + return false; + } + // LIFO removal is safe only for our sole pending entry; all host queue writers share the sequencer. + if (pending.items.length > 0 || steering.length !== 1 || steering[0] !== submitted.pendingMessage.message.text) { + throw new Error('Cannot replace pending steering while the SDK has other pending work'); + } + return (await this._wrapper.session.rpc.queue.removeMostRecent()).removed; + } + async getMessages(): Promise { const result = await this._getMappedEvents(); return result.turns; @@ -3622,11 +3693,11 @@ export class CopilotAgentSession extends Disposable { async setModel(model: string, reasoningEffort?: SessionConfig['reasoningEffort'], contextTier?: SessionConfig['contextTier'], autoTier?: AutoModeTier | null): Promise { this._logService.info(`[Copilot:${this.sessionId}] Changing model to: ${model}`); - await this._awaitControlPlaneRpc('session.setModel', this._wrapper.session.setModel(model, { + await this._awaitControlPlaneRpc('session.setModel', this._messageQueueSequencer.queue(() => this._wrapper.session.setModel(model, { reasoningEffort, contextTier, ...(autoTier !== undefined ? { autoTier } : {}), - })); + }))); this._lastSeenModelId = model; } @@ -5109,7 +5180,7 @@ export class CopilotAgentSession extends Disposable { // be associated with the root turn boundary. // // 2. If the content matches a steering message we acknowledged - // via {@link sendSteering}, promote it to its own protocol + // via {@link setPendingSteering}, promote it to its own protocol // turn (closing the in-flight turn) BEFORE step 3 so the // event id is recorded against the new steering turn rather // than the preempted one. @@ -6871,6 +6942,7 @@ export class CopilotAgentSession extends Disposable { this._register(wrapper.onPendingMessagesModified(() => { this._logService.trace(`[Copilot:${sessionId}] Pending messages modified`); + void this._syncPendingSteering(); })); this._register(wrapper.onBackgroundTasksChanged(() => { diff --git a/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts b/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts index c6b60e4fa37bed..db27eb9578f36e 100644 --- a/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts +++ b/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts @@ -107,9 +107,12 @@ class MockCopilotSession { readonly sessionId = 'test-session-1'; readonly sendRequests: unknown[] = []; readonly sendMessagesRequests: unknown[] = []; + readonly pendingSteeringMessages: string[] = []; + readonly pendingQueueItems: Awaited>['items'] = []; sendMessagesError: Error | undefined; sendMessagesGate: Promise | undefined; sendGate: Promise | undefined; + inFlightSteeringCount = 0; readonly modeSetCalls: Array<{ mode: 'interactive' | 'plan' | 'autopilot' }> = []; readonly permissionModeSetCalls: PermissionMode[] = []; permissionModeSetSuccess = true; @@ -294,14 +297,21 @@ class MockCopilotSession { } // Stubs for methods the wrapper / session class calls - async send(request: unknown) { + async send(request: Parameters[0]) { this.operationLog.push('send'); this.sendRequests.push(request); + const message = typeof request === 'string' ? { prompt: request } : request; + if (message.mode === 'immediate') { + this.pendingSteeringMessages.push(message.displayPrompt ?? message.prompt); + } await this.sendGate; return `message-${this.sendRequests.length}`; } async abort() { this.abortCalls++; + this.pendingSteeringMessages.length = 0; + this.pendingQueueItems.length = 0; + this.inFlightSteeringCount = 0; await this.abortGate; } async setModel(...args: Parameters) { @@ -330,6 +340,33 @@ class MockCopilotSession { } await this.sendMessagesGate; }, + queue: { + pendingItems: async () => { + this.operationLog.push('queue.pendingItems'); + return { + items: this.pendingQueueItems.slice(), + steeringMessages: this.pendingSteeringMessages.slice(), + inFlightSteeringCount: this.inFlightSteeringCount, + }; + }, + removeAt: async ({ id }: Parameters[0]) => { + this.operationLog.push('queue.removeAt'); + const index = this.pendingQueueItems.findIndex(item => item.id === id); + if (index === -1) { + return { removed: false }; + } + this.pendingQueueItems.splice(index, 1); + return { removed: true }; + }, + removeMostRecent: async () => { + this.operationLog.push('queue.removeMostRecent'); + if (this.pendingSteeringMessages.length <= this.inFlightSteeringCount) { + return { removed: false }; + } + this.pendingSteeringMessages.pop(); + return { removed: true }; + }, + }, debug: { collectLogs: async (params: Parameters[0]) => { this.collectLogsCalls.push(params); @@ -6975,15 +7012,15 @@ suite('CopilotAgentSession', () => { }); }); - // ---- sendSteering ---- + // ---- setPendingSteering ---- - suite('sendSteering', () => { + suite('setPendingSteering', () => { test('forwards attachments to the SDK', async () => { const { session, mockSession } = await createAgentSession(disposables); const imageUri = URI.file('/session/attachments/pasted-image.png'); - await session.sendSteering({ + await session.setPendingSteering({ id: 'steer-1', message: { text: 'see the screenshot', @@ -6999,6 +7036,7 @@ suite('CopilotAgentSession', () => { assert.deepStrictEqual(mockSession.sendRequests, [{ prompt: 'see the screenshot', + displayPrompt: 'see the screenshot', attachments: [{ type: 'file', path: imageUri.fsPath, @@ -7012,7 +7050,7 @@ suite('CopilotAgentSession', () => { const snapshotUri = URI.file('/session/attachments/pasted.txt'); const { session, mockSession } = await createAgentSession(disposables); - await session.sendSteering({ + await session.setPendingSteering({ id: 'steer-text', message: { text: 'use this', @@ -7032,6 +7070,7 @@ suite('CopilotAgentSession', () => { // the attachment keeps its plain display name. assert.deepStrictEqual(mockSession.sendRequests, [{ prompt: `use this\n\n\n${expectedSnapshotReadonlyNote([snapshotUri.fsPath])}\n`, + displayPrompt: 'use this', attachments: [{ type: 'file', path: snapshotUri.fsPath, @@ -7045,7 +7084,7 @@ suite('CopilotAgentSession', () => { const { session, mockSession, signals } = await createAgentSession(disposables); session.resetTurnState('turn-original'); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); // Sending the steering must not flip turns until the SDK has // echoed the user message back through the event stream. @@ -7072,7 +7111,7 @@ suite('CopilotAgentSession', () => { session.resetTurnState('turn-original'); const imageUri = URI.file('/session/attachments/pasted-image.png'); - await session.sendSteering({ + await session.setPendingSteering({ id: 'steer-attachment', message: { text: 'Inspect the attached screenshot.', @@ -7131,7 +7170,7 @@ Use the attached image as context. return sendGate.p; }; - const steeringPromise = session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + const steeringPromise = session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); await timeout(0); assert.strictEqual(mockSession.sendRequests.length, 1); @@ -7164,7 +7203,7 @@ Use the attached image as context. return sendGate.p; }; - const steeringPromise = session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + const steeringPromise = session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); await timeout(0); assert.strictEqual(mockSession.sendRequests.length, 1); mockSession.fire('session.idle', {} as SessionEventPayload<'session.idle'>['data']); @@ -7194,7 +7233,7 @@ Use the attached image as context. const { session, mockSession, signals } = await createAgentSession(disposables); session.resetTurnState('turn-original'); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'focus on tests', interactionId: 'interaction-steer', @@ -7233,7 +7272,7 @@ Use the attached image as context. toolRequests: [{ toolCallId: 'tc-1', name: 'grep', arguments: {} }], } as SessionEventPayload<'assistant.message'>['data']); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'focus on tests', interactionId: 'interaction-steer', @@ -7267,7 +7306,7 @@ Use the attached image as context. const { session, mockSession, signals } = await createAgentSession(disposables); session.resetTurnState('turn-original'); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); // SDK injects an unrelated user.message (e.g. skill content) // with the steering's exact text but a non-'user' source. @@ -7287,7 +7326,7 @@ Use the attached image as context. const { session, mockSession, signals } = await createAgentSession(disposables, { sessionDatabase }); session.resetTurnState('turn-original'); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'focus on tests', } as SessionEventPayload<'user.message'>['data'], { agentId: 'agent-1', id: 'evt-subagent' }); @@ -7307,7 +7346,7 @@ Use the attached image as context. const { session, mockSession, signals } = await createAgentSession(disposables); session.resetTurnState('turn-original'); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'something completely different', } as SessionEventPayload<'user.message'>['data']); @@ -7319,16 +7358,261 @@ Use the attached image as context. test('does not send the same steering message again before it is flipped', async () => { const { session, mockSession } = await createAgentSession(disposables); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); assert.strictEqual(mockSession.sendRequests.length, 1); }); + test('replaces edited steering messages before the SDK consumes them', async () => { + const { session, mockSession } = await createAgentSession(disposables); + + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-2', message: { text: 'focus on tests and telemetry', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-3', message: { text: 'focus on tests, telemetry, and product logic', origin: { kind: MessageKind.User } } }); + + assert.deepStrictEqual({ + operations: mockSession.operationLog.filter(operation => operation === 'send' || operation.startsWith('queue.')), + pendingSteeringMessages: mockSession.pendingSteeringMessages, + }, { + operations: [ + 'send', + 'queue.pendingItems', + 'queue.removeMostRecent', + 'send', + 'queue.pendingItems', + 'queue.removeMostRecent', + 'send', + ], + pendingSteeringMessages: ['focus on tests, telemetry, and product logic'], + }); + }); + + test('coalesces the remove and add actions produced by editing a pending steering message', async () => { + const { session, mockSession } = await createAgentSession(disposables); + + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + const remove = session.setPendingSteering(undefined); + const replace = session.setPendingSteering({ id: 'steer-2', message: { text: 'focus on tests and telemetry', origin: { kind: MessageKind.User } } }); + await Promise.all([remove, replace]); + + assert.deepStrictEqual({ + operations: mockSession.operationLog.filter(operation => operation === 'send' || operation.startsWith('queue.')), + pendingSteeringMessages: mockSession.pendingSteeringMessages, + }, { + operations: [ + 'send', + 'queue.pendingItems', + 'queue.removeMostRecent', + 'send', + ], + pendingSteeringMessages: ['focus on tests and telemetry'], + }); + }); + + test('sends the latest edit after the previous steering message was already consumed', async () => { + const { session, mockSession } = await createAgentSession(disposables); + + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + mockSession.inFlightSteeringCount = 1; + await session.setPendingSteering({ id: 'steer-2', message: { text: 'focus on tests and telemetry', origin: { kind: MessageKind.User } } }); + mockSession.fire('user.message', { + content: 'focus on tests', + interactionId: 'interaction-steer', + } as SessionEventPayload<'user.message'>['data']); + await timeout(0); + + assert.deepStrictEqual({ + operations: mockSession.operationLog.filter(operation => operation === 'send' || operation.startsWith('queue.')), + pendingSteeringMessages: mockSession.pendingSteeringMessages, + }, { + operations: [ + 'send', + 'queue.pendingItems', + 'send', + ], + pendingSteeringMessages: ['focus on tests', 'focus on tests and telemetry'], + }); + }); + + test('updates a pending steering message without changing its protocol id', async () => { + const { session, mockSession, signals } = await createAgentSession(disposables); + session.resetTurnState('turn-original'); + + await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'final revision', origin: { kind: MessageKind.User } } }); + for (const content of mockSession.pendingSteeringMessages) { + mockSession.fire('user.message', { content }); + } + + assert.deepStrictEqual(getActions(signals) + .filter(action => action.type === ActionType.ChatTurnStarted) + .map(action => ({ id: action.queuedMessageId, text: action.message.text })), [ + { id: 'steer-1', text: 'final revision' }, + ]); + }); + + test('removes an attachment-only pending steering message', async () => { + const { session, mockSession } = await createAgentSession(disposables); + await session.setPendingSteering({ + id: 'steer-1', + message: { + text: '', + origin: { kind: MessageKind.User }, + attachments: [{ type: MessageAttachmentKind.Simple, label: 'context', modelRepresentation: 'sample context' }], + }, + }); + + await session.setPendingSteering(undefined); + + assert.deepStrictEqual(mockSession.pendingSteeringMessages, []); + }); + + test('coalesces edits while the previous SDK send is still pending', async () => { + const { session, mockSession } = await createAgentSession(disposables); + const sendGate = new DeferredPromise(); + mockSession.sendGate = sendGate.p; + const first = session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); + await timeout(0); + const second = session.setPendingSteering({ id: 'steer-2', message: { text: 'intermediate revision', origin: { kind: MessageKind.User } } }); + const third = session.setPendingSteering({ id: 'steer-3', message: { text: 'final revision', origin: { kind: MessageKind.User } } }); + sendGate.complete(); + await Promise.all([first, second, third]); + + assert.deepStrictEqual({ + sentPrompts: mockSession.sendRequests.map(request => (request as { prompt: string }).prompt), + pending: mockSession.pendingSteeringMessages, + }, { + sentPrompts: ['first revision', 'final revision'], + pending: ['final revision'], + }); + }); + + test('does not remove steering that was consumed while reading the SDK queue', async () => { + const { session, mockSession } = await createAgentSession(disposables); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); + const pendingItems = mockSession.rpc.queue.pendingItems; + mockSession.rpc.queue.pendingItems = async () => { + const snapshot = await pendingItems(); + mockSession.inFlightSteeringCount = 1; + mockSession.fire('user.message', { content: 'first revision' }); + return snapshot; + }; + + await session.setPendingSteering({ id: 'steer-2', message: { text: 'final revision', origin: { kind: MessageKind.User } } }); + + assert.deepStrictEqual({ + removed: mockSession.operationLog.includes('queue.removeMostRecent'), + pending: mockSession.pendingSteeringMessages.slice(mockSession.inFlightSteeringCount), + }, { + removed: false, + pending: ['final revision'], + }); + }); + + test('serializes normal sends behind pending steering removal', async () => { + const { session, mockSession } = await createAgentSession(disposables); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); + const gate = new DeferredPromise(); + const pendingItems = mockSession.rpc.queue.pendingItems; + mockSession.rpc.queue.pendingItems = async () => { + const snapshot = await pendingItems(); + await gate.p; + return snapshot; + }; + const remove = session.setPendingSteering(undefined); + await timeout(0); + const send = session.send('next turn', undefined, 'turn-next'); + await timeout(0); + const sendsBeforeRemoval = mockSession.sendRequests.length; + gate.complete(); + await Promise.all([remove, send]); + + assert.deepStrictEqual({ + sendsBeforeRemoval, + operations: mockSession.operationLog.filter(operation => operation === 'send' || operation.startsWith('queue.')), + }, { + sendsBeforeRemoval: 1, + operations: ['send', 'queue.pendingItems', 'queue.removeMostRecent', 'send'], + }); + }); + + test('removes steering by stable id when the SDK promotes it to the normal queue', async () => { + const { session, mockSession } = await createAgentSession(disposables); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); + mockSession.pendingSteeringMessages.length = 0; + mockSession.pendingQueueItems.push( + { id: 'queue-1', messageId: 'message-1', kind: 'message', displayText: 'first revision', agentMode: 'interactive' }, + { id: 'queue-2', messageId: 'other-message', kind: 'message', displayText: 'unrelated work', agentMode: 'interactive' }, + ); + + await session.setPendingSteering(undefined); + + assert.deepStrictEqual({ + pendingIds: mockSession.pendingQueueItems.map(item => item.id), + removedMostRecent: mockSession.operationLog.includes('queue.removeMostRecent'), + }, { + pendingIds: ['queue-2'], + removedMostRecent: false, + }); + }); + + test('preserves unrelated SDK work and retries when the queue changes', async () => { + const logService = new CapturingLogService(); + const { session, mockSession } = await createAgentSession(disposables, { logService }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); + mockSession.pendingQueueItems.push({ id: 'other', kind: 'command', displayText: '/model', agentMode: 'interactive' }); + + await session.setPendingSteering({ id: 'steer-2', message: { text: 'final revision', origin: { kind: MessageKind.User } } }); + const beforeRetry = { + pending: mockSession.pendingSteeringMessages.slice(), + removedMostRecent: mockSession.operationLog.includes('queue.removeMostRecent'), + errors: logService.errors.length, + }; + mockSession.pendingQueueItems.length = 0; + mockSession.fire('pending_messages.modified', {}); + await timeout(0); + + assert.deepStrictEqual({ + beforeRetry, + pending: mockSession.pendingSteeringMessages, + }, { + beforeRetry: { pending: ['first revision'], removedMostRecent: false, errors: 1 }, + pending: ['final revision'], + }); + }); + + for (const cleanup of ['abort', 'dispose'] as const) { + test(`does not send a replacement after ${cleanup} during queue reconciliation`, async () => { + const { session, mockSession } = await createAgentSession(disposables); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); + const gate = new DeferredPromise(); + const pendingItems = mockSession.rpc.queue.pendingItems; + mockSession.rpc.queue.pendingItems = async () => { + const snapshot = await pendingItems(); + await gate.p; + return snapshot; + }; + const edit = session.setPendingSteering({ id: 'steer-2', message: { text: 'final revision', origin: { kind: MessageKind.User } } }); + await timeout(0); + await session[cleanup](); + gate.complete(); + await edit; + + assert.deepStrictEqual({ + sends: mockSession.sendRequests.length, + removedMostRecent: mockSession.operationLog.includes('queue.removeMostRecent'), + }, { + sends: 1, + removedMostRecent: false, + }); + }); + } + test('fires steering_consumed on abort when the steering never reached its turn', async () => { const { session, signals } = await createAgentSession(disposables); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); await session.abort(); const consumed = signals.find(s => s.kind === 'steering_consumed'); @@ -7346,7 +7630,7 @@ Use the attached image as context. session.resetTurnState('turn-original'); mockSession.fire('assistant.turn_start', { turnId: 'sdk-0' } as SessionEventPayload<'assistant.turn_start'>['data']); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'focus on tests', interactionId: 'interaction-steer', @@ -7375,7 +7659,7 @@ Use the attached image as context. session.resetTurnState('turn-original'); mockSession.fire('assistant.turn_start', { turnId: 'sdk-0', interactionId: 'interaction-original' }); - await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'focus on tests', interactionId: interactionId ? 'interaction-steer' : undefined, @@ -7442,7 +7726,7 @@ Use the attached image as context. mockSession.send = async () => { throw new Error('send failed'); }; - await session.sendSteering({ id: 'steer-fail', message: { text: 'will fail', origin: { kind: MessageKind.User } } }); + await session.setPendingSteering({ id: 'steer-fail', message: { text: 'will fail', origin: { kind: MessageKind.User } } }); const consumed = signals.find(s => s.kind === 'steering_consumed'); const turnStarted = signals.find(s => s.kind === 'action' && (s as IAgentActionSignal).action.type === ActionType.ChatTurnStarted); From 0fd516924efd4fec1081db3c2a918aedf5b99d87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joaqu=C3=ADn=20Ruales?= <1588988+jruales@users.noreply.github.com> Date: Sun, 20 Sep 2026 12:47:25 -0700 Subject: [PATCH 2/4] Disable editing pending steering messages Replace SDK queue reconciliation with shared UI editing guards. Preserve queued and sent request editing and restore the provider's existing send, cancel, and model-switch behavior. Fixes #325884 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../agentHost/node/copilot/copilotAgent.ts | 5 +- .../node/copilot/copilotAgentSession.ts | 150 +++----- .../test/node/copilotAgentSession.test.ts | 324 ++---------------- .../browser/actions/chatAccessibilityHelp.ts | 1 + .../chat/browser/actions/chatQueueActions.ts | 6 +- .../browser/chatEditing/chatEditingActions.ts | 6 +- .../chat/browser/widget/chatListRenderer.ts | 9 +- .../contrib/chat/browser/widget/chatWidget.ts | 8 +- .../chat/common/actions/chatContextKeys.ts | 1 + .../chat/common/model/chatViewModel.ts | 5 + .../chatAccessibilityHelp.test.ts | 8 + .../browser/actions/chatQueueActions.test.ts | 81 ++++- .../browser/widget/chatListRenderer.test.ts | 78 +++++ .../test/browser/widget/chatWidget.test.ts | 27 +- .../test/common/model/chatViewModel.test.ts | 14 +- 15 files changed, 290 insertions(+), 433 deletions(-) diff --git a/src/vs/platform/agentHost/node/copilot/copilotAgent.ts b/src/vs/platform/agentHost/node/copilot/copilotAgent.ts index a9cf050ed5308a..cf1879240cfff4 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotAgent.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotAgent.ts @@ -4341,7 +4341,10 @@ export class CopilotAgent extends Disposable implements IAgent { return; } - void target.setPendingSteering(steeringMessage); + // Steering: send with mode 'immediate' so the SDK injects it mid-turn + if (steeringMessage) { + target.sendSteering(steeringMessage); + } // Queued messages are consumed by the server (AgentSideEffects) // which dispatches ChatTurnStarted and calls sendMessage directly. diff --git a/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts b/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts index 2813021ac1d35d..03f3625620f9eb 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts @@ -1085,9 +1085,7 @@ export class CopilotAgentSession extends Disposable { private readonly _sandboxConfigSequencer = new Sequencer(); private readonly _mcpEnablementSequencer = new Sequencer(); private readonly _mcpServerLifecycleSequencer = new SequencerByKey(); - private readonly _messageQueueSequencer = new Sequencer(); - private _desiredPendingSteering: PendingMessage | undefined; - private _submittedPendingSteering: { pendingMessage: PendingMessage; sdkMessageId?: string } | undefined; + private readonly _steeringMessagesInFlight = new Set(); /** * Steering messages that have been accepted by the SDK but not yet * surfaced to the chat UI as a separate user message. When the SDK @@ -1456,14 +1454,17 @@ export class CopilotAgentSession extends Disposable { return newTurnId; } - /** Clears pending steering on abort or disposal, including edits still being prepared. */ + /** + * Drains any steering messages we acknowledged to the SDK but never + * promoted to their own turn (e.g. on abort or session dispose). Fires + * `steering_consumed` so the chat UI removes the lingering pending + * steering bubble even when no fresh `user.message` arrives. + */ private _drainPendingSteeringFlips(): void { - const ids = new Set(this._pendingSteeringFlips.keys()); - if (this._desiredPendingSteering) { - ids.add(this._desiredPendingSteering.id); + if (this._pendingSteeringFlips.size === 0) { + return; } - this._desiredPendingSteering = undefined; - this._submittedPendingSteering = undefined; + const ids = [...this._pendingSteeringFlips.keys()]; this._pendingSteeringFlips.clear(); for (const id of ids) { this._onDidSessionProgress.fire({ @@ -1491,7 +1492,6 @@ export class CopilotAgentSession extends Disposable { for (const [id, msg] of this._pendingSteeringFlips) { if (msg.message.text === content) { this._pendingSteeringFlips.delete(id); - this._didConsumePendingSteering(msg); return msg; } if (msg.message.text.length > 0 @@ -1502,23 +1502,11 @@ export class CopilotAgentSession extends Disposable { } if (substringMatch) { this._pendingSteeringFlips.delete(substringMatch[0]); - this._didConsumePendingSteering(substringMatch[1]); return substringMatch[1]; } return undefined; } - private _didConsumePendingSteering(steeringMessage: PendingMessage): void { - if (!equals(this._submittedPendingSteering?.pendingMessage, steeringMessage)) { - return; - } - this._submittedPendingSteering = undefined; - if (equals(this._desiredPendingSteering, steeringMessage)) { - this._desiredPendingSteering = undefined; - } - void this._syncPendingSteering(); - } - private _parentToolCallIdForSubagentEvent(e: { readonly agentId?: string }): string | undefined { return e.agentId ? this._parentToolCallIdsByAgentId.get(e.agentId) : undefined; } @@ -3013,10 +3001,10 @@ export class CopilotAgentSession extends Disposable { await this.applyMode(mode); let result: CopilotCommandInvocationResult; try { - result = await this._messageQueueSequencer.queue(() => this._wrapper.session.rpc.commands.invoke({ + result = await this._wrapper.session.rpc.commands.invoke({ name: runtimeSlashCommand.name, ...(slashCommand.rawRest.length > 0 ? { input: slashCommand.rawRest } : {}), - })); + }); } catch (err) { this._logService.error(err, `[Copilot:${this.sessionId}] rpc.commands.invoke(${slashCommand.command}) failed`); throw err; @@ -3070,7 +3058,7 @@ export class CopilotAgentSession extends Disposable { const sendingTurn = this._currentTurn.value; sendingTurn?.markProviderCallPending(); try { - await this._messageQueueSequencer.queue(() => this._otelService.withTraceContext(traceContext, async () => { + await this._otelService.withTraceContext(traceContext, () => { if (!this._environmentService.isBuilt && prompt === '$error') { return this._wrapper.session.rpc.sendMessages({ messages: [{ prompt }], @@ -3078,7 +3066,7 @@ export class CopilotAgentSession extends Disposable { }); } return this._wrapper.session.send({ prompt, attachments: sdkAttachments?.length ? sdkAttachments : undefined }); - })); + }); sendingTurn?.markProviderCallResolved(); } catch (error) { sendingTurn?.markProviderCallRejected(); @@ -3446,94 +3434,35 @@ export class CopilotAgentSession extends Disposable { return this._configurationService.getRootValue(platformRootSchema, AgentHostAutoReplyEnabledConfigKey) === true; } - /** Synchronizes the single protocol steering message with the SDK's pending steering queue. */ - setPendingSteering(steeringMessage: PendingMessage | undefined): Promise { - this._desiredPendingSteering = steeringMessage; - return this._syncPendingSteering(); - } - - private _syncPendingSteering(): Promise { - const token = this._abortToken; - return this._messageQueueSequencer.queue(() => this._reconcilePendingSteering(token)).catch(err => { - this._logService.error(`[Copilot:${this.sessionId}] Failed to synchronize pending steering`, err); - }); - } - - private async _reconcilePendingSteering(token: CancellationToken): Promise { - while (!token.isCancellationRequested && !this.isDisposed && !equals(this._submittedPendingSteering?.pendingMessage, this._desiredPendingSteering)) { - const submitted = this._submittedPendingSteering; - if (submitted) { - if (!await this._removePendingSteering(submitted, token)) { - return; - } - this._pendingSteeringFlips.delete(submitted.pendingMessage.id); - if (this._submittedPendingSteering === submitted) { - this._submittedPendingSteering = undefined; - } - } - - const desired = this._desiredPendingSteering; - if (!desired) { - return; - } - + async sendSteering(steeringMessage: PendingMessage): Promise { + if (this._steeringMessagesInFlight.has(steeringMessage.id) || this._pendingSteeringFlips.has(steeringMessage.id)) { + return; + } + this._steeringMessagesInFlight.add(steeringMessage.id); + this._logService.info(`[Copilot:${this.sessionId}] Sending steering message: "${steeringMessage.message.text.substring(0, 100)}"`); + try { await this._reconcileMcpServerEnablement(); - if (token.isCancellationRequested || this.isDisposed || !equals(this._desiredPendingSteering, desired)) { - continue; - } - const sdkAttachments = await this._toSdkAttachments(desired.message.attachments); - if (token.isCancellationRequested || this.isDisposed || !equals(this._desiredPendingSteering, desired)) { - continue; - } + this._pendingSteeringFlips.set(steeringMessage.id, steeringMessage); + const sdkAttachments = await this._toSdkAttachments(steeringMessage.message.attachments); // Steering is injected into the active turn and never fires the SDK's `user-prompt-submitted` // hook, so the read-only snapshot signal can't ride `additionalContext` here. Fold it into the // prompt as a `` block instead: the runtime forwards it to the model, and the host's // `stripPromptScaffolding` removes it from the displayed message (#331154). - const snapshotReminder = this._snapshotReadonlyReminder(desired.message.attachments); + const snapshotReminder = this._snapshotReadonlyReminder(steeringMessage.message.attachments); const steeringPrompt = snapshotReminder - ? `${desired.message.text}\n\n\n${snapshotReminder}\n` - : desired.message.text; - const sending: NonNullable = { pendingMessage: desired }; - this._submittedPendingSteering = sending; - this._pendingSteeringFlips.set(desired.id, desired); - try { - sending.sdkMessageId = await this._wrapper.session.send({ - prompt: steeringPrompt, - displayPrompt: desired.message.text, - attachments: sdkAttachments?.length ? sdkAttachments : undefined, - mode: 'immediate', - }); - } catch (err) { - this._pendingSteeringFlips.delete(desired.id); - if (this._submittedPendingSteering === sending) { - this._submittedPendingSteering = undefined; - } - throw err; - } - } - } - - private async _removePendingSteering(submitted: NonNullable, token: CancellationToken): Promise { - const pending = await this._wrapper.session.rpc.queue.pendingItems(); - if (token.isCancellationRequested || this.isDisposed) { - return false; - } - if (this._submittedPendingSteering !== submitted) { - return true; - } - const queued = pending.items.find(item => submitted.sdkMessageId !== undefined && item.messageId === submitted.sdkMessageId); - if (queued) { - return (await this._wrapper.session.rpc.queue.removeAt({ id: queued.id })).removed; - } - const steering = pending.steeringMessages.slice(pending.inFlightSteeringCount ?? 0); - if (steering.length === 0) { - return false; - } - // LIFO removal is safe only for our sole pending entry; all host queue writers share the sequencer. - if (pending.items.length > 0 || steering.length !== 1 || steering[0] !== submitted.pendingMessage.message.text) { - throw new Error('Cannot replace pending steering while the SDK has other pending work'); + ? `${steeringMessage.message.text}\n\n\n${snapshotReminder}\n` + : steeringMessage.message.text; + await this._wrapper.session.send({ + prompt: steeringPrompt, + attachments: sdkAttachments?.length ? sdkAttachments : undefined, + mode: 'immediate', + }); + } catch (err) { + this._pendingSteeringFlips.delete(steeringMessage.id); + this._logService.error(`[Copilot:${this.sessionId}] Steering message failed`, err); + } finally { + this._steeringMessagesInFlight.delete(steeringMessage.id); } - return (await this._wrapper.session.rpc.queue.removeMostRecent()).removed; } async getMessages(): Promise { @@ -3693,11 +3622,11 @@ export class CopilotAgentSession extends Disposable { async setModel(model: string, reasoningEffort?: SessionConfig['reasoningEffort'], contextTier?: SessionConfig['contextTier'], autoTier?: AutoModeTier | null): Promise { this._logService.info(`[Copilot:${this.sessionId}] Changing model to: ${model}`); - await this._awaitControlPlaneRpc('session.setModel', this._messageQueueSequencer.queue(() => this._wrapper.session.setModel(model, { + await this._awaitControlPlaneRpc('session.setModel', this._wrapper.session.setModel(model, { reasoningEffort, contextTier, ...(autoTier !== undefined ? { autoTier } : {}), - }))); + })); this._lastSeenModelId = model; } @@ -5180,7 +5109,7 @@ export class CopilotAgentSession extends Disposable { // be associated with the root turn boundary. // // 2. If the content matches a steering message we acknowledged - // via {@link setPendingSteering}, promote it to its own protocol + // via {@link sendSteering}, promote it to its own protocol // turn (closing the in-flight turn) BEFORE step 3 so the // event id is recorded against the new steering turn rather // than the preempted one. @@ -6942,7 +6871,6 @@ export class CopilotAgentSession extends Disposable { this._register(wrapper.onPendingMessagesModified(() => { this._logService.trace(`[Copilot:${sessionId}] Pending messages modified`); - void this._syncPendingSteering(); })); this._register(wrapper.onBackgroundTasksChanged(() => { diff --git a/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts b/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts index db27eb9578f36e..c6b60e4fa37bed 100644 --- a/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts +++ b/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts @@ -107,12 +107,9 @@ class MockCopilotSession { readonly sessionId = 'test-session-1'; readonly sendRequests: unknown[] = []; readonly sendMessagesRequests: unknown[] = []; - readonly pendingSteeringMessages: string[] = []; - readonly pendingQueueItems: Awaited>['items'] = []; sendMessagesError: Error | undefined; sendMessagesGate: Promise | undefined; sendGate: Promise | undefined; - inFlightSteeringCount = 0; readonly modeSetCalls: Array<{ mode: 'interactive' | 'plan' | 'autopilot' }> = []; readonly permissionModeSetCalls: PermissionMode[] = []; permissionModeSetSuccess = true; @@ -297,21 +294,14 @@ class MockCopilotSession { } // Stubs for methods the wrapper / session class calls - async send(request: Parameters[0]) { + async send(request: unknown) { this.operationLog.push('send'); this.sendRequests.push(request); - const message = typeof request === 'string' ? { prompt: request } : request; - if (message.mode === 'immediate') { - this.pendingSteeringMessages.push(message.displayPrompt ?? message.prompt); - } await this.sendGate; return `message-${this.sendRequests.length}`; } async abort() { this.abortCalls++; - this.pendingSteeringMessages.length = 0; - this.pendingQueueItems.length = 0; - this.inFlightSteeringCount = 0; await this.abortGate; } async setModel(...args: Parameters) { @@ -340,33 +330,6 @@ class MockCopilotSession { } await this.sendMessagesGate; }, - queue: { - pendingItems: async () => { - this.operationLog.push('queue.pendingItems'); - return { - items: this.pendingQueueItems.slice(), - steeringMessages: this.pendingSteeringMessages.slice(), - inFlightSteeringCount: this.inFlightSteeringCount, - }; - }, - removeAt: async ({ id }: Parameters[0]) => { - this.operationLog.push('queue.removeAt'); - const index = this.pendingQueueItems.findIndex(item => item.id === id); - if (index === -1) { - return { removed: false }; - } - this.pendingQueueItems.splice(index, 1); - return { removed: true }; - }, - removeMostRecent: async () => { - this.operationLog.push('queue.removeMostRecent'); - if (this.pendingSteeringMessages.length <= this.inFlightSteeringCount) { - return { removed: false }; - } - this.pendingSteeringMessages.pop(); - return { removed: true }; - }, - }, debug: { collectLogs: async (params: Parameters[0]) => { this.collectLogsCalls.push(params); @@ -7012,15 +6975,15 @@ suite('CopilotAgentSession', () => { }); }); - // ---- setPendingSteering ---- + // ---- sendSteering ---- - suite('setPendingSteering', () => { + suite('sendSteering', () => { test('forwards attachments to the SDK', async () => { const { session, mockSession } = await createAgentSession(disposables); const imageUri = URI.file('/session/attachments/pasted-image.png'); - await session.setPendingSteering({ + await session.sendSteering({ id: 'steer-1', message: { text: 'see the screenshot', @@ -7036,7 +6999,6 @@ suite('CopilotAgentSession', () => { assert.deepStrictEqual(mockSession.sendRequests, [{ prompt: 'see the screenshot', - displayPrompt: 'see the screenshot', attachments: [{ type: 'file', path: imageUri.fsPath, @@ -7050,7 +7012,7 @@ suite('CopilotAgentSession', () => { const snapshotUri = URI.file('/session/attachments/pasted.txt'); const { session, mockSession } = await createAgentSession(disposables); - await session.setPendingSteering({ + await session.sendSteering({ id: 'steer-text', message: { text: 'use this', @@ -7070,7 +7032,6 @@ suite('CopilotAgentSession', () => { // the attachment keeps its plain display name. assert.deepStrictEqual(mockSession.sendRequests, [{ prompt: `use this\n\n\n${expectedSnapshotReadonlyNote([snapshotUri.fsPath])}\n`, - displayPrompt: 'use this', attachments: [{ type: 'file', path: snapshotUri.fsPath, @@ -7084,7 +7045,7 @@ suite('CopilotAgentSession', () => { const { session, mockSession, signals } = await createAgentSession(disposables); session.resetTurnState('turn-original'); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); // Sending the steering must not flip turns until the SDK has // echoed the user message back through the event stream. @@ -7111,7 +7072,7 @@ suite('CopilotAgentSession', () => { session.resetTurnState('turn-original'); const imageUri = URI.file('/session/attachments/pasted-image.png'); - await session.setPendingSteering({ + await session.sendSteering({ id: 'steer-attachment', message: { text: 'Inspect the attached screenshot.', @@ -7170,7 +7131,7 @@ Use the attached image as context. return sendGate.p; }; - const steeringPromise = session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + const steeringPromise = session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); await timeout(0); assert.strictEqual(mockSession.sendRequests.length, 1); @@ -7203,7 +7164,7 @@ Use the attached image as context. return sendGate.p; }; - const steeringPromise = session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + const steeringPromise = session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); await timeout(0); assert.strictEqual(mockSession.sendRequests.length, 1); mockSession.fire('session.idle', {} as SessionEventPayload<'session.idle'>['data']); @@ -7233,7 +7194,7 @@ Use the attached image as context. const { session, mockSession, signals } = await createAgentSession(disposables); session.resetTurnState('turn-original'); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'focus on tests', interactionId: 'interaction-steer', @@ -7272,7 +7233,7 @@ Use the attached image as context. toolRequests: [{ toolCallId: 'tc-1', name: 'grep', arguments: {} }], } as SessionEventPayload<'assistant.message'>['data']); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'focus on tests', interactionId: 'interaction-steer', @@ -7306,7 +7267,7 @@ Use the attached image as context. const { session, mockSession, signals } = await createAgentSession(disposables); session.resetTurnState('turn-original'); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); // SDK injects an unrelated user.message (e.g. skill content) // with the steering's exact text but a non-'user' source. @@ -7326,7 +7287,7 @@ Use the attached image as context. const { session, mockSession, signals } = await createAgentSession(disposables, { sessionDatabase }); session.resetTurnState('turn-original'); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'focus on tests', } as SessionEventPayload<'user.message'>['data'], { agentId: 'agent-1', id: 'evt-subagent' }); @@ -7346,7 +7307,7 @@ Use the attached image as context. const { session, mockSession, signals } = await createAgentSession(disposables); session.resetTurnState('turn-original'); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'something completely different', } as SessionEventPayload<'user.message'>['data']); @@ -7358,261 +7319,16 @@ Use the attached image as context. test('does not send the same steering message again before it is flipped', async () => { const { session, mockSession } = await createAgentSession(disposables); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); assert.strictEqual(mockSession.sendRequests.length, 1); }); - test('replaces edited steering messages before the SDK consumes them', async () => { - const { session, mockSession } = await createAgentSession(disposables); - - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); - await session.setPendingSteering({ id: 'steer-2', message: { text: 'focus on tests and telemetry', origin: { kind: MessageKind.User } } }); - await session.setPendingSteering({ id: 'steer-3', message: { text: 'focus on tests, telemetry, and product logic', origin: { kind: MessageKind.User } } }); - - assert.deepStrictEqual({ - operations: mockSession.operationLog.filter(operation => operation === 'send' || operation.startsWith('queue.')), - pendingSteeringMessages: mockSession.pendingSteeringMessages, - }, { - operations: [ - 'send', - 'queue.pendingItems', - 'queue.removeMostRecent', - 'send', - 'queue.pendingItems', - 'queue.removeMostRecent', - 'send', - ], - pendingSteeringMessages: ['focus on tests, telemetry, and product logic'], - }); - }); - - test('coalesces the remove and add actions produced by editing a pending steering message', async () => { - const { session, mockSession } = await createAgentSession(disposables); - - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); - const remove = session.setPendingSteering(undefined); - const replace = session.setPendingSteering({ id: 'steer-2', message: { text: 'focus on tests and telemetry', origin: { kind: MessageKind.User } } }); - await Promise.all([remove, replace]); - - assert.deepStrictEqual({ - operations: mockSession.operationLog.filter(operation => operation === 'send' || operation.startsWith('queue.')), - pendingSteeringMessages: mockSession.pendingSteeringMessages, - }, { - operations: [ - 'send', - 'queue.pendingItems', - 'queue.removeMostRecent', - 'send', - ], - pendingSteeringMessages: ['focus on tests and telemetry'], - }); - }); - - test('sends the latest edit after the previous steering message was already consumed', async () => { - const { session, mockSession } = await createAgentSession(disposables); - - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); - mockSession.inFlightSteeringCount = 1; - await session.setPendingSteering({ id: 'steer-2', message: { text: 'focus on tests and telemetry', origin: { kind: MessageKind.User } } }); - mockSession.fire('user.message', { - content: 'focus on tests', - interactionId: 'interaction-steer', - } as SessionEventPayload<'user.message'>['data']); - await timeout(0); - - assert.deepStrictEqual({ - operations: mockSession.operationLog.filter(operation => operation === 'send' || operation.startsWith('queue.')), - pendingSteeringMessages: mockSession.pendingSteeringMessages, - }, { - operations: [ - 'send', - 'queue.pendingItems', - 'send', - ], - pendingSteeringMessages: ['focus on tests', 'focus on tests and telemetry'], - }); - }); - - test('updates a pending steering message without changing its protocol id', async () => { - const { session, mockSession, signals } = await createAgentSession(disposables); - session.resetTurnState('turn-original'); - - await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'final revision', origin: { kind: MessageKind.User } } }); - for (const content of mockSession.pendingSteeringMessages) { - mockSession.fire('user.message', { content }); - } - - assert.deepStrictEqual(getActions(signals) - .filter(action => action.type === ActionType.ChatTurnStarted) - .map(action => ({ id: action.queuedMessageId, text: action.message.text })), [ - { id: 'steer-1', text: 'final revision' }, - ]); - }); - - test('removes an attachment-only pending steering message', async () => { - const { session, mockSession } = await createAgentSession(disposables); - await session.setPendingSteering({ - id: 'steer-1', - message: { - text: '', - origin: { kind: MessageKind.User }, - attachments: [{ type: MessageAttachmentKind.Simple, label: 'context', modelRepresentation: 'sample context' }], - }, - }); - - await session.setPendingSteering(undefined); - - assert.deepStrictEqual(mockSession.pendingSteeringMessages, []); - }); - - test('coalesces edits while the previous SDK send is still pending', async () => { - const { session, mockSession } = await createAgentSession(disposables); - const sendGate = new DeferredPromise(); - mockSession.sendGate = sendGate.p; - const first = session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); - await timeout(0); - const second = session.setPendingSteering({ id: 'steer-2', message: { text: 'intermediate revision', origin: { kind: MessageKind.User } } }); - const third = session.setPendingSteering({ id: 'steer-3', message: { text: 'final revision', origin: { kind: MessageKind.User } } }); - sendGate.complete(); - await Promise.all([first, second, third]); - - assert.deepStrictEqual({ - sentPrompts: mockSession.sendRequests.map(request => (request as { prompt: string }).prompt), - pending: mockSession.pendingSteeringMessages, - }, { - sentPrompts: ['first revision', 'final revision'], - pending: ['final revision'], - }); - }); - - test('does not remove steering that was consumed while reading the SDK queue', async () => { - const { session, mockSession } = await createAgentSession(disposables); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); - const pendingItems = mockSession.rpc.queue.pendingItems; - mockSession.rpc.queue.pendingItems = async () => { - const snapshot = await pendingItems(); - mockSession.inFlightSteeringCount = 1; - mockSession.fire('user.message', { content: 'first revision' }); - return snapshot; - }; - - await session.setPendingSteering({ id: 'steer-2', message: { text: 'final revision', origin: { kind: MessageKind.User } } }); - - assert.deepStrictEqual({ - removed: mockSession.operationLog.includes('queue.removeMostRecent'), - pending: mockSession.pendingSteeringMessages.slice(mockSession.inFlightSteeringCount), - }, { - removed: false, - pending: ['final revision'], - }); - }); - - test('serializes normal sends behind pending steering removal', async () => { - const { session, mockSession } = await createAgentSession(disposables); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); - const gate = new DeferredPromise(); - const pendingItems = mockSession.rpc.queue.pendingItems; - mockSession.rpc.queue.pendingItems = async () => { - const snapshot = await pendingItems(); - await gate.p; - return snapshot; - }; - const remove = session.setPendingSteering(undefined); - await timeout(0); - const send = session.send('next turn', undefined, 'turn-next'); - await timeout(0); - const sendsBeforeRemoval = mockSession.sendRequests.length; - gate.complete(); - await Promise.all([remove, send]); - - assert.deepStrictEqual({ - sendsBeforeRemoval, - operations: mockSession.operationLog.filter(operation => operation === 'send' || operation.startsWith('queue.')), - }, { - sendsBeforeRemoval: 1, - operations: ['send', 'queue.pendingItems', 'queue.removeMostRecent', 'send'], - }); - }); - - test('removes steering by stable id when the SDK promotes it to the normal queue', async () => { - const { session, mockSession } = await createAgentSession(disposables); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); - mockSession.pendingSteeringMessages.length = 0; - mockSession.pendingQueueItems.push( - { id: 'queue-1', messageId: 'message-1', kind: 'message', displayText: 'first revision', agentMode: 'interactive' }, - { id: 'queue-2', messageId: 'other-message', kind: 'message', displayText: 'unrelated work', agentMode: 'interactive' }, - ); - - await session.setPendingSteering(undefined); - - assert.deepStrictEqual({ - pendingIds: mockSession.pendingQueueItems.map(item => item.id), - removedMostRecent: mockSession.operationLog.includes('queue.removeMostRecent'), - }, { - pendingIds: ['queue-2'], - removedMostRecent: false, - }); - }); - - test('preserves unrelated SDK work and retries when the queue changes', async () => { - const logService = new CapturingLogService(); - const { session, mockSession } = await createAgentSession(disposables, { logService }); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); - mockSession.pendingQueueItems.push({ id: 'other', kind: 'command', displayText: '/model', agentMode: 'interactive' }); - - await session.setPendingSteering({ id: 'steer-2', message: { text: 'final revision', origin: { kind: MessageKind.User } } }); - const beforeRetry = { - pending: mockSession.pendingSteeringMessages.slice(), - removedMostRecent: mockSession.operationLog.includes('queue.removeMostRecent'), - errors: logService.errors.length, - }; - mockSession.pendingQueueItems.length = 0; - mockSession.fire('pending_messages.modified', {}); - await timeout(0); - - assert.deepStrictEqual({ - beforeRetry, - pending: mockSession.pendingSteeringMessages, - }, { - beforeRetry: { pending: ['first revision'], removedMostRecent: false, errors: 1 }, - pending: ['final revision'], - }); - }); - - for (const cleanup of ['abort', 'dispose'] as const) { - test(`does not send a replacement after ${cleanup} during queue reconciliation`, async () => { - const { session, mockSession } = await createAgentSession(disposables); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'first revision', origin: { kind: MessageKind.User } } }); - const gate = new DeferredPromise(); - const pendingItems = mockSession.rpc.queue.pendingItems; - mockSession.rpc.queue.pendingItems = async () => { - const snapshot = await pendingItems(); - await gate.p; - return snapshot; - }; - const edit = session.setPendingSteering({ id: 'steer-2', message: { text: 'final revision', origin: { kind: MessageKind.User } } }); - await timeout(0); - await session[cleanup](); - gate.complete(); - await edit; - - assert.deepStrictEqual({ - sends: mockSession.sendRequests.length, - removedMostRecent: mockSession.operationLog.includes('queue.removeMostRecent'), - }, { - sends: 1, - removedMostRecent: false, - }); - }); - } - test('fires steering_consumed on abort when the steering never reached its turn', async () => { const { session, signals } = await createAgentSession(disposables); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); await session.abort(); const consumed = signals.find(s => s.kind === 'steering_consumed'); @@ -7630,7 +7346,7 @@ Use the attached image as context. session.resetTurnState('turn-original'); mockSession.fire('assistant.turn_start', { turnId: 'sdk-0' } as SessionEventPayload<'assistant.turn_start'>['data']); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'focus on tests', interactionId: 'interaction-steer', @@ -7659,7 +7375,7 @@ Use the attached image as context. session.resetTurnState('turn-original'); mockSession.fire('assistant.turn_start', { turnId: 'sdk-0', interactionId: 'interaction-original' }); - await session.setPendingSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-1', message: { text: 'focus on tests', origin: { kind: MessageKind.User } } }); mockSession.fire('user.message', { content: 'focus on tests', interactionId: interactionId ? 'interaction-steer' : undefined, @@ -7726,7 +7442,7 @@ Use the attached image as context. mockSession.send = async () => { throw new Error('send failed'); }; - await session.setPendingSteering({ id: 'steer-fail', message: { text: 'will fail', origin: { kind: MessageKind.User } } }); + await session.sendSteering({ id: 'steer-fail', message: { text: 'will fail', origin: { kind: MessageKind.User } } }); const consumed = signals.find(s => s.kind === 'steering_consumed'); const turnStarted = signals.find(s => s.kind === 'action' && (s as IAgentActionSignal).action.type === ActionType.ChatTurnStarted); diff --git a/src/vs/workbench/contrib/chat/browser/actions/chatAccessibilityHelp.ts b/src/vs/workbench/contrib/chat/browser/actions/chatAccessibilityHelp.ts index 5c1fbcd892288b..6647267bbdafda 100644 --- a/src/vs/workbench/contrib/chat/browser/actions/chatAccessibilityHelp.ts +++ b/src/vs/workbench/contrib/chat/browser/actions/chatAccessibilityHelp.ts @@ -79,6 +79,7 @@ export function getAccessibilityHelpText(type: 'panelChat' | 'inlineChat' | 'qui content.push(localize('chat.modelPicker.pricingDetails', "Pricing Details expands in place without moving the model's controls. Expansion and collapse are immediate when reduced motion is enabled. If the details exceed the available space, use Page Up or Page Down while the model details have focus to scroll.")); content.push(localize('chat.modelPicker.search', "Type while the model list is focused to search across all providers. In the search field, use Up and Down Arrow to navigate results, Enter to select a model, and Escape to close the picker. Left and Right Arrow move the text cursor.")); content.push(localize('chat.fileChangesDisclosure', 'File change summaries show the total files, additions, and deletions. Focus the disclosure and press Enter or Space to show or hide the individual files. Focus an additions and deletions label and press Enter or Space to open the changes in a diff editor.')); + content.push(localize('chat.pendingRequestEditing', "Queued messages can be edited before they are sent. Pending steering messages cannot be edited because they may already have been submitted to the agent.")); } if (type === 'panelChat' || type === 'quickChat' || type === 'agentView') { if (type === 'quickChat') { diff --git a/src/vs/workbench/contrib/chat/browser/actions/chatQueueActions.ts b/src/vs/workbench/contrib/chat/browser/actions/chatQueueActions.ts index 1f9f9e3ad06973..87c8946471e5f3 100644 --- a/src/vs/workbench/contrib/chat/browser/actions/chatQueueActions.ts +++ b/src/vs/workbench/contrib/chat/browser/actions/chatQueueActions.ts @@ -17,7 +17,7 @@ import { ChatContextKeys } from '../../common/actions/chatContextKeys.js'; import { ChatRequestQueueKind, IChatService } from '../../common/chatService/chatService.js'; import { IChatSideChatService } from '../../common/chatSideChatService.js'; import { ChatConfiguration } from '../../common/constants.js'; -import { isRequestVM } from '../../common/model/chatViewModel.js'; +import { isEditableRequestVM, isRequestVM } from '../../common/model/chatViewModel.js'; import { IChatWidgetService } from '../chat.js'; import { captureSideChatSelection } from '../chatSideChat.js'; import { CHAT_CATEGORY } from './chatActions.js'; @@ -278,7 +278,7 @@ export class ChatEditPendingRequestAction extends Action2 { group: 'navigation', order: 2, when: ContextKeyExpr.and( - ChatContextKeys.isRequest, + ChatContextKeys.isEditableRequest, ChatContextKeys.isPendingRequest, ContextKeyExpr.notEquals(`config.${ChatConfiguration.EditRequests}`, 'hover'), ContextKeyExpr.notEquals(`config.${ChatConfiguration.EditRequests}`, 'input') @@ -291,7 +291,7 @@ export class ChatEditPendingRequestAction extends Action2 { const widgetService = accessor.get(IChatWidgetService); const [context] = args; - if (!isRequestVM(context) || !context.pendingKind) { + if (!isEditableRequestVM(context) || !context.pendingKind) { return; } diff --git a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts index 0f404817b5b860..2a4bf4c5ba87c3 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts @@ -31,7 +31,7 @@ import { isChatViewTitleActionContext } from '../../common/actions/chatActions.j import { ChatContextKeyExprs, ChatContextKeys } from '../../common/actions/chatContextKeys.js'; import { applyingChatEditsFailedContextKey, CHAT_EDITING_MULTI_DIFF_SOURCE_RESOLVER_SCHEME, chatEditingResourceContextKey, chatEditingWidgetFileStateContextKey, decidedChatEditingResourceContextKey, hasAppliedChatEditsContextKey, hasUndecidedChatEditingResourceContextKey, IChatEditingService, IChatEditingSession, ModifiedFileEntryState } from '../../common/editing/chatEditingService.js'; import { IChatService } from '../../common/chatService/chatService.js'; -import { isChatTreeItem, isRequestVM, isResponseVM } from '../../common/model/chatViewModel.js'; +import { isChatTreeItem, isEditableRequestVM, isRequestVM, isResponseVM } from '../../common/model/chatViewModel.js'; import { ChatAgentLocation, ChatConfiguration, ChatModeKind } from '../../common/constants.js'; import { CHAT_CATEGORY } from '../actions/chatActions.js'; import { ChatTreeItem, IChatWidget, IChatWidgetService } from '../chat.js'; @@ -671,7 +671,7 @@ registerAction2(class EditAction extends Action2 { id: MenuId.ChatMessageTitle, group: 'navigation', order: 2, - when: ContextKeyExpr.and(ContextKeyExpr.or(ContextKeyExpr.equals(`config.${ChatConfiguration.EditRequests}`, 'hover'), ContextKeyExpr.equals(`config.${ChatConfiguration.EditRequests}`, 'input')), ChatContextKeys.readOnly.negate()) + when: ContextKeyExpr.and(ContextKeyExpr.or(ContextKeyExpr.equals(`config.${ChatConfiguration.EditRequests}`, 'hover'), ContextKeyExpr.equals(`config.${ChatConfiguration.EditRequests}`, 'input')), ChatContextKeys.readOnly.negate(), ChatContextKeys.isEditableRequest) } ] }); @@ -689,7 +689,7 @@ registerAction2(class EditAction extends Action2 { return; } - if (isRequestVM(item)) { + if (isEditableRequestVM(item)) { widget?.startEditing(item.id); } } diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts b/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts index 321fa60fa5e785..cc115ac399200e 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts @@ -62,7 +62,7 @@ import { ChatQuestionCarouselData } from '../../common/model/chatProgressTypes/c import { localChatSessionType, SessionType } from '../../common/chatSessionsService.js'; import { getChatSessionType } from '../../common/model/chatUri.js'; import { getExplicitFileOrImageAttachmentSummary, IChatRequestVariableEntry, isExplicitFileOrImageVariableEntry, isPasteVariableEntry } from '../../common/attachments/chatVariableEntries.js'; -import { getStickyScrollTargetItem, IChatChangesSummaryPart, IChatCodeCitations, IChatErrorDetailsPart, IChatReferences, IChatRendererContent, IChatRequestViewModel, IChatResponseViewModel, IChatViewModel, IChatWorkingProgress, isRequestVM, isResponseVM, IChatPendingDividerViewModel, isPendingDividerVM, IChatTurnPillsPart } from '../../common/model/chatViewModel.js'; +import { getStickyScrollTargetItem, IChatChangesSummaryPart, IChatCodeCitations, IChatErrorDetailsPart, IChatReferences, IChatRendererContent, IChatRequestViewModel, IChatResponseViewModel, IChatViewModel, IChatWorkingProgress, isEditableRequestVM, isRequestVM, isResponseVM, IChatPendingDividerViewModel, isPendingDividerVM, IChatTurnPillsPart } from '../../common/model/chatViewModel.js'; import { getNWords } from '../../common/model/chatWordCounter.js'; import { CHAT_OPEN_AGENT_HOST_CHAT_COMMAND_ID, ChatAgentLocation, ChatConfiguration, ChatModeKind, ChatProgressAnimation, CollapsedToolsDisplayMode, ThinkingDisplayMode } from '../../common/constants.js'; import { getConfiguredProgressAnimation } from './chatWorkingLogo.js'; @@ -1473,6 +1473,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer('chat.editRequests') !== 'none' && this.rendererOptions.editable) { + if (this.configService.getValue('chat.editRequests') !== 'none' && this.rendererOptions.editable && isEditableRequestVM(element)) { templateData.elementDisposables.add(dom.addDisposableListener(templateData.rowContainer, dom.EventType.KEY_DOWN, e => { const ev = new StandardKeyboardEvent(e); if (ev.equals(KeyCode.Space) || ev.equals(KeyCode.Enter)) { @@ -2471,7 +2472,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer('chat.editRequests') === 'inline' && this.rendererOptions.editable) { + if (this.configService.getValue('chat.editRequests') === 'inline' && this.rendererOptions.editable && isEditableRequestVM(element)) { container.classList.add('clickable'); store.add(dom.addDisposableListener(container, dom.EventType.CLICK, (e: MouseEvent) => { if (this.viewModel?.editing?.id === element.id) { @@ -5151,7 +5152,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer this.fireItemHeightChange(templateData))); if (isRequestVM(element)) { markdownPart.domNode.tabIndex = 0; - if (this.configService.getValue('chat.editRequests') === 'inline' && this.rendererOptions.editable) { + if (this.configService.getValue('chat.editRequests') === 'inline' && this.rendererOptions.editable && isEditableRequestVM(element)) { markdownPart.domNode.classList.add('clickable'); markdownPart.addDisposable(dom.addDisposableListener(markdownPart.domNode, dom.EventType.CLICK, (e: MouseEvent) => { if (this.viewModel?.editing?.id === element.id) { diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts b/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts index 7ed6a9bbcc194d..626d51b17a60e5 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts @@ -71,7 +71,7 @@ import { IChatSessionsService, localChatSessionType } from '../../common/chatSes import { IChatSlashCommandService } from '../../common/participants/chatSlashCommands.js'; import { IChatTodoListService } from '../../common/tools/chatTodoListService.js'; import { ChatRequestVariableSet, IChatRequestTranscriptContextVariableEntry, IChatRequestVariableEntry, isPastedTextArtifact, isPromptFileVariableEntry, isPromptTextVariableEntry, isWorkspaceVariableEntry, PromptFileVariableKind, toPromptFileVariableEntry } from '../../common/attachments/chatVariableEntries.js'; -import { ChatViewModel, IChatResponseViewModel, isRequestVM, isResponseVM } from '../../common/model/chatViewModel.js'; +import { ChatViewModel, IChatResponseViewModel, isEditableRequestVM, isRequestVM, isResponseVM } from '../../common/model/chatViewModel.js'; import { ChatMessageRole, IChatMessage } from '../../common/languageModels.js'; import { ChatAgentLocation, ChatConfiguration, ChatModeKind, ChatPermissionLevel, IResolvedNewChatSessionType, ThinkingDisplayMode } from '../../common/constants.js'; import { IChatGoalSummaryService } from '../chatGoalSummaryService.js'; @@ -2224,7 +2224,11 @@ export class ChatWidget extends Disposable implements IChatWidget { private clickedRequest(item: IChatListItemTemplate) { const currentElement = item.currentElement; - if (isRequestVM(currentElement) && !this.viewModel?.editing) { + if (!isEditableRequestVM(currentElement)) { + return; + } + + if (!this.viewModel?.editing) { const requests = this.viewModel?.model.getRequests(); if (!requests || !this.viewModel?.sessionResource) { diff --git a/src/vs/workbench/contrib/chat/common/actions/chatContextKeys.ts b/src/vs/workbench/contrib/chat/common/actions/chatContextKeys.ts index 7241af0630240c..68b18eab30a463 100644 --- a/src/vs/workbench/contrib/chat/common/actions/chatContextKeys.ts +++ b/src/vs/workbench/contrib/chat/common/actions/chatContextKeys.ts @@ -37,6 +37,7 @@ export namespace ChatContextKeys { export const contextMenuIsBackground = new RawContextKey('chatContextMenuIsBackground', false, { type: 'boolean', description: localize('chatContextMenuIsBackground', "Whether the chat context menu was opened from the transcript background rather than chat item content.") }); export const isFirstRequest = new RawContextKey('chatFirstRequest', false, { type: 'boolean', description: localize('chatFirstRequest', "The chat item is the first request in the session.") }); export const isPendingRequest = new RawContextKey('chatRequestIsPending', false, { type: 'boolean', description: localize('chatRequestIsPending', "True when the chat request item is pending in the queue.") }); + export const isEditableRequest = new RawContextKey('chatRequestIsEditable', false, { type: 'boolean', description: localize('chatRequestIsEditable', "True when the chat request item can be edited.") }); export const itemId = new RawContextKey('chatItemId', '', { type: 'string', description: localize('chatItemId', "The id of the chat item.") }); export const lastItemId = new RawContextKey('chatLastItemId', [], { type: 'string', description: localize('chatLastItemId', "The id of the last chat item.") }); diff --git a/src/vs/workbench/contrib/chat/common/model/chatViewModel.ts b/src/vs/workbench/contrib/chat/common/model/chatViewModel.ts index 91df3055f3c507..6efa509738030b 100644 --- a/src/vs/workbench/contrib/chat/common/model/chatViewModel.ts +++ b/src/vs/workbench/contrib/chat/common/model/chatViewModel.ts @@ -24,6 +24,11 @@ export function isRequestVM(item: unknown): item is IChatRequestViewModel { return !!item && typeof item === 'object' && 'message' in item; } +/** Pending steering may already be in the agent's input queue and cannot be safely replaced. */ +export function isEditableRequestVM(item: unknown): item is IChatRequestViewModel { + return isRequestVM(item) && item.pendingKind !== ChatRequestQueueKind.Steering; +} + export function isResponseVM(item: unknown): item is IChatResponseViewModel { return !!item && typeof (item as IChatResponseViewModel).setVote !== 'undefined'; } diff --git a/src/vs/workbench/contrib/chat/test/browser/accessibility/chatAccessibilityHelp.test.ts b/src/vs/workbench/contrib/chat/test/browser/accessibility/chatAccessibilityHelp.test.ts index 3beb999e4f65df..4a1f1bbb03e9e5 100644 --- a/src/vs/workbench/contrib/chat/test/browser/accessibility/chatAccessibilityHelp.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/accessibility/chatAccessibilityHelp.test.ts @@ -14,6 +14,14 @@ import { AGENT_SESSION_RENAME_ACTION_ID } from '../../../browser/agentSessions/a suite('Chat Accessibility Help', () => { ensureNoDisposablesAreLeakedInTestSuite(); + test('distinguishes editable queued messages from pending steering', () => { + const help = getAccessibilityHelpText('agentView', new MockKeybindingService(), true); + assert.deepStrictEqual({ + queued: help.includes('Queued messages can be edited before they are sent'), + steering: help.includes('Pending steering messages cannot be edited'), + }, { queued: true, steering: true }); + }); + test('documents accepting the selected confirmation primary action', () => { const help = getAccessibilityHelpText('agentView', new MockKeybindingService(), true); assert.deepStrictEqual({ diff --git a/src/vs/workbench/contrib/chat/test/browser/actions/chatQueueActions.test.ts b/src/vs/workbench/contrib/chat/test/browser/actions/chatQueueActions.test.ts index f70612f72d1b52..9f42efd30e7e10 100644 --- a/src/vs/workbench/contrib/chat/test/browser/actions/chatQueueActions.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/actions/chatQueueActions.test.ts @@ -12,6 +12,8 @@ import { URI } from '../../../../../../base/common/uri.js'; import { upcastPartial } from '../../../../../../base/test/common/mock.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js'; import { ICodeEditor } from '../../../../../../editor/browser/editorBrowser.js'; +import { isIMenuItem, MenuId, MenuRegistry } from '../../../../../../platform/actions/common/actions.js'; +import { CommandsRegistry } from '../../../../../../platform/commands/common/commands.js'; import { TestConfigurationService } from '../../../../../../platform/configuration/test/common/testConfigurationService.js'; import { ContextKeyService } from '../../../../../../platform/contextkey/browser/contextKeyService.js'; import { TestInstantiationService } from '../../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; @@ -23,17 +25,92 @@ import { ILogService, NullLogService } from '../../../../../../platform/log/comm import { INotificationService } from '../../../../../../platform/notification/common/notification.js'; import { TestNotificationService } from '../../../../../../platform/notification/test/common/testNotificationService.js'; import { IChatWidget, IChatWidgetService } from '../../../browser/chat.js'; -import { ChatAskInSideChatAction, ChatQueueMessageAction, ChatSteerWithMessageAction, registerChatQueueActions } from '../../../browser/actions/chatQueueActions.js'; +import { ChatAskInSideChatAction, ChatEditPendingRequestAction, ChatQueueMessageAction, ChatRemovePendingRequestAction, ChatSendPendingImmediatelyAction, ChatSteerWithMessageAction, registerChatQueueActions } from '../../../browser/actions/chatQueueActions.js'; +import '../../../browser/chatEditing/chatEditingActions.js'; import { ChatContextKeys } from '../../../common/actions/chatContextKeys.js'; import { IChatSideChatService } from '../../../common/chatSideChatService.js'; import { ChatConfiguration } from '../../../common/constants.js'; import { IChatModel, IChatRequestModel } from '../../../common/model/chatModel.js'; -import { IChatViewModel } from '../../../common/model/chatViewModel.js'; +import { IChatRequestViewModel, IChatViewModel } from '../../../common/model/chatViewModel.js'; import { ChatRequestQueueKind } from '../../../common/chatService/chatService.js'; // Register actions once so the keybindings appear in KeybindingsRegistry. registerChatQueueActions(); +suite('Pending request editing actions', () => { + const disposables = ensureNoDisposablesAreLeakedInTestSuite(); + const editRequestId = 'workbench.action.chat.editRequests'; + + for (const editMode of ['inline', 'hover', 'input']) { + test(`hides steering edit actions in ${editMode} mode without hiding other queue actions`, () => { + const config = new TestConfigurationService({ [ChatConfiguration.EditRequests]: editMode }); + const contextKeyService = disposables.add(new ContextKeyService(config)); + const menuItems = MenuRegistry.getMenuItems(MenuId.ChatMessageTitle).filter(isIMenuItem); + const visibleActions = (pendingKind: ChatRequestQueueKind | undefined) => { + const context = contextKeyService.createOverlay([ + [ChatContextKeys.isRequest.key, true], + [ChatContextKeys.isPendingRequest.key, pendingKind !== undefined], + [ChatContextKeys.isEditableRequest.key, pendingKind !== ChatRequestQueueKind.Steering], + ]); + const actions = menuItems.filter(item => context.contextMatchesRules(item.when)).map(item => item.command.id); + return { + edit: actions.filter(id => id === editRequestId || id === ChatEditPendingRequestAction.ID), + remove: actions.includes(ChatRemovePendingRequestAction.ID), + send: actions.includes(ChatSendPendingImmediatelyAction.ID), + }; + }; + + assert.deepStrictEqual({ + steering: visibleActions(ChatRequestQueueKind.Steering), + queued: visibleActions(ChatRequestQueueKind.Queued), + sent: visibleActions(undefined), + }, { + steering: { edit: [], remove: true, send: true }, + queued: { edit: [editMode === 'inline' ? ChatEditPendingRequestAction.ID : editRequestId], remove: true, send: true }, + sent: { edit: editMode === 'inline' ? [] : [editRequestId], remove: false, send: false }, + }); + }); + } + + test('guards command and keyboard editing while preserving queued and sent editing', async () => { + const instantiationService = disposables.add(new TestInstantiationService()); + const sessionResource = URI.parse('test:///session'); + const editedRequests: string[] = []; + let focusedRequest: IChatRequestViewModel | undefined; + const widget = upcastPartial({ + startEditing: id => editedRequests.push(id), + getFocus: () => focusedRequest, + }); + instantiationService.stub(IChatWidgetService, upcastPartial({ + getWidgetBySessionResource: () => widget, + lastFocusedWidget: widget, + })); + const pendingAction = new ChatEditPendingRequestAction(); + const editCommand = CommandsRegistry.getCommand(editRequestId); + assert.ok(editCommand); + + for (const pendingKind of [ChatRequestQueueKind.Steering, ChatRequestQueueKind.Queued, undefined]) { + focusedRequest = upcastPartial({ + id: pendingKind ?? 'sent', + sessionResource, + message: { text: 'request', parts: [] }, + pendingKind, + }); + instantiationService.invokeFunction(accessor => pendingAction.run(accessor, focusedRequest)); + await instantiationService.invokeFunction(accessor => editCommand.handler(accessor, focusedRequest)); + await instantiationService.invokeFunction(accessor => editCommand.handler(accessor)); + } + + assert.deepStrictEqual(editedRequests, [ + ChatRequestQueueKind.Queued, + ChatRequestQueueKind.Queued, + ChatRequestQueueKind.Queued, + 'sent', + 'sent', + ]); + }); +}); + suite('Queue/Steer keybinding resolution', () => { ensureNoDisposablesAreLeakedInTestSuite(); diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/chatListRenderer.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/chatListRenderer.test.ts index b6d7eaa7bab423..51c1b90d09c67d 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/chatListRenderer.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/chatListRenderer.test.ts @@ -39,6 +39,7 @@ import { ChatInputPart } from '../../../browser/widget/input/chatInputPart.js'; import { ChatToolConfirmationCarouselPart } from '../../../browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationCarouselPart.js'; import { ChatSubagentContentPart } from '../../../browser/widget/chatContentParts/chatSubagentContentPart.js'; import { OpenSubagentChatActionViewItem } from '../../../browser/widget/chatContentParts/chatSubagentOpenChat.js'; +import { ChatContextKeys } from '../../../common/actions/chatContextKeys.js'; import { ChatThinkingContentPart } from '../../../browser/widget/chatContentParts/chatThinkingContentPart.js'; import { ChatMarkdownContentPart } from '../../../browser/widget/chatContentParts/chatMarkdownContentPart.js'; import { aggregateChatEditDiffs } from '../../../browser/widget/chatContentParts/chatEditStatsButton.js'; @@ -817,6 +818,83 @@ suite('ChatListRenderer', () => { }); }); + for (const sticky of [false, true]) { + test(`pending steering has no mouse or keyboard edit affordances${sticky ? ' in sticky scroll' : ''}`, async () => { + const disposables = store.add(new DisposableStore()); + const instantiationService = workbenchInstantiationService(undefined, disposables); + const configurationService = new TestConfigurationService(); + await configurationService.setUserConfiguration(ChatConfiguration.EditRequests, 'inline'); + await configurationService.setUserConfiguration(ChatConfiguration.CheckpointsEnabled, false); + instantiationService.stub(IConfigurationService, configurationService); + instantiationService.stub(IChatService, new MockChatService()); + instantiationService.stub(IChatModelFeedbackSurveyService, new MockChatModelFeedbackSurveyService()); + instantiationService.stub(IChatAgentService, disposables.add(instantiationService.createInstance(ChatAgentService))); + + const model = disposables.add(instantiationService.createInstance(ChatModel, undefined, { initialLocation: ChatAgentLocation.Chat, canUseTools: true })); + const viewModel = disposables.add(instantiationService.createInstance(ChatViewModel, model, undefined)); + const text = 'request'; + const request = model.addRequest({ + text, + parts: [new ChatRequestTextPart(new OffsetRange(0, text.length), new Range(1, 1, 1, text.length + 1), text)] + }, { variables: [] }, Date.now()); + const container = mainWindow.document.createElement('div'); + container.classList.toggle('monaco-tree-sticky-row', sticky); + mainWindow.document.body.appendChild(container); + disposables.add(toDisposable(() => container.remove())); + const renderer = disposables.add(instantiationService.createInstance( + ChatListItemRenderer, + {} as ChatEditorOptions, + { editable: true }, + { + getListLength: () => 1, + container, + currentChatMode: () => ChatModeKind.Agent, + isStickyScrollEnabled: () => sticky, + refreshStickyScroll: () => { }, + stickyScrollTopPadding: 0, + }, + undefined, + viewModel, + )); + const template = renderer.renderTemplate(container); + disposables.add(toDisposable(() => renderer.disposeTemplate(template))); + let editEvents = 0; + disposables.add(renderer.onDidClickRequest(() => editEvents++)); + const states = []; + + for (const pendingKind of [ChatRequestQueueKind.Queued, ChatRequestQueueKind.Steering, undefined]) { + model.removePendingRequest(request.id); + if (pendingKind !== undefined) { + model.addPendingRequest(request, pendingKind, {}); + } + const requestViewModel = viewModel.getItems().filter(isRequestVM).find(item => item.pendingKind === pendingKind); + assert.ok(requestViewModel); + const node = { element: requestViewModel, children: [], depth: 0, visibleChildrenCount: 0, visibleChildIndex: 0, collapsible: false, collapsed: false, visible: true, filterData: undefined }; + renderer.renderElement(node, 0, template); + const markdown = template.value.querySelector('.rendered-markdown'); + assert.ok(markdown); + editEvents = 0; + markdown.click(); + for (const keyCode of [13, 32]) { + markdown.dispatchEvent(new mainWindow.KeyboardEvent('keydown', { keyCode, bubbles: true, cancelable: true })); + } + states.push({ + pendingKind, + editable: template.contextKeyService.getContextKeyValue(ChatContextKeys.isEditableRequest.key), + clickable: markdown.classList.contains('clickable'), + editEvents, + }); + renderer.disposeElement(node, 0, template); + } + + assert.deepStrictEqual(states, [ + { pendingKind: ChatRequestQueueKind.Queued, editable: true, clickable: true, editEvents: 3 }, + { pendingKind: ChatRequestQueueKind.Steering, editable: false, clickable: false, editEvents: 0 }, + { pendingKind: undefined, editable: true, clickable: true, editEvents: 3 }, + ]); + }); + } + test('pending divider clears a timestamp from a recycled request template', () => { const disposables = store.add(new DisposableStore()); const instantiationService = workbenchInstantiationService(undefined, disposables); diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/chatWidget.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/chatWidget.test.ts index f62979319f45fe..7fcab22ce4d514 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/chatWidget.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/chatWidget.test.ts @@ -156,7 +156,7 @@ suite('ChatWidget', () => { }]); }); - test('editing a steering request passes its model and configuration to the input', async () => { + test('editing a queued request passes its model and configuration to the input', async () => { const modelId = 'agent-host-copilot:claude-opus-4.8'; const modelConfiguration = { reasoningEffort: 'xhigh' }; const configurationService = new TestConfigurationService(); @@ -178,7 +178,7 @@ suite('ChatWidget', () => { variables: [], modelId, modelConfiguration, - pendingKind: ChatRequestQueueKind.Steering, + pendingKind: ChatRequestQueueKind.Queued, }); let editing: IChatRequestViewModel | undefined; const widget = Object.create(ChatWidget.prototype) as ChatWidget; @@ -212,6 +212,29 @@ suite('ChatWidget', () => { assert.deepStrictEqual(input.requestModelByIdentifier.firstCall.args, [modelId, modelConfiguration]); }); + test('does not start editing a pending steering request', () => { + const request = upcastPartial({ + id: 'steering-request', + message: { text: 'original steering', parts: [] }, + pendingKind: ChatRequestQueueKind.Steering, + }); + const widget = Object.create(ChatWidget.prototype) as ChatWidget; + Object.defineProperties(widget, { + viewModel: { + value: { + model: { getRequests: () => assert.fail('Editing pending steering must not touch the model') }, + }, + }, + listWidget: { + value: { getTemplateDataForRequestId: () => ({ currentElement: request }) }, + }, + }); + + widget.startEditing(request.id); + + assert.strictEqual(widget.viewModel?.editing, undefined); + }); + test('confirms before cancelling changed request edits', async () => { const scenarios = [ { name: 'unchanged', input: 'original request', attachmentIds: ['original-attachment'] }, diff --git a/src/vs/workbench/contrib/chat/test/common/model/chatViewModel.test.ts b/src/vs/workbench/contrib/chat/test/common/model/chatViewModel.test.ts index 8b77609e8d3435..6e199e009569ce 100644 --- a/src/vs/workbench/contrib/chat/test/common/model/chatViewModel.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/model/chatViewModel.test.ts @@ -6,7 +6,7 @@ import assert from 'assert'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js'; import { ChatRequestQueueKind } from '../../../common/chatService/chatService.js'; -import { getStickyScrollTargetItem } from '../../../common/model/chatViewModel.js'; +import { getStickyScrollTargetItem, isEditableRequestVM } from '../../../common/model/chatViewModel.js'; interface ITestChatViewModelItem { readonly id: string; @@ -17,6 +17,18 @@ interface ITestChatViewModelItem { suite('ChatViewModel', () => { ensureNoDisposablesAreLeakedInTestSuite(); + test('pending steering requests are not editable', () => { + const message = { text: 'request', parts: [] }; + + assert.deepStrictEqual([ + isEditableRequestVM({ message }), + isEditableRequestVM({ message, pendingKind: ChatRequestQueueKind.Queued }), + isEditableRequestVM({ message, pendingKind: ChatRequestQueueKind.Steering }), + isEditableRequestVM({ kind: 'pendingDivider' }), + isEditableRequestVM(undefined), + ], [true, true, false, false, false]); + }); + test('sticky scroll target ignores trailing pending items', () => { const response: ITestChatViewModelItem = { id: 'response' }; const pendingOnly: ITestChatViewModelItem = { id: 'pending-only', pendingKind: ChatRequestQueueKind.Queued }; From 393c8e5bcd23176edb69bad13581ca38a822409f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joaqu=C3=ADn=20Ruales?= <1588988+jruales@users.noreply.github.com> Date: Sun, 20 Sep 2026 14:38:40 -0700 Subject: [PATCH 3/4] Refresh chat rows when pending request kind changes Include the pending kind in request rendering identity so queued-to-steering transitions refresh edit controls and listeners. Cover both transition directions through the real chat list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../chat/common/model/chatViewModel.ts | 2 +- .../browser/widget/chatListWidget.test.ts | 56 ++++++++++++++++++- 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/contrib/chat/common/model/chatViewModel.ts b/src/vs/workbench/contrib/chat/common/model/chatViewModel.ts index 6efa509738030b..f8e9ae29185035 100644 --- a/src/vs/workbench/contrib/chat/common/model/chatViewModel.ts +++ b/src/vs/workbench/contrib/chat/common/model/chatViewModel.ts @@ -430,7 +430,7 @@ class ChatRequestViewModel implements IChatRequestViewModel { * An ID that changes when the request should be re-rendered. */ get dataId() { - return `${this.id}_${this._model.version + (this._model.response?.isComplete ? 1 : 0)}`; + return `${this.id}_${this._model.version + (this._model.response?.isComplete ? 1 : 0)}_${this._pendingKind ?? ''}`; } get sessionResource() { diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/chatListWidget.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/chatListWidget.test.ts index db85137745fc90..a1e630b5c8882d 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/chatListWidget.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/chatListWidget.test.ts @@ -24,10 +24,10 @@ import { IChatAccessibilityService } from '../../../browser/chat.js'; import { ChatAttachmentWidgetRegistry, IChatAttachmentWidgetRegistry } from '../../../browser/attachments/chatAttachmentWidgetRegistry.js'; import { computeScrollDownState, getAnchoredScrollTop, AutoScrollHolds, UserToggleResizeState, ChatListWidget, IChatListWidgetOptions, getChatContextMenuTargetContext, isChatBackgroundContextMenuTarget } from '../../../browser/widget/chatListWidget.js'; import { ChatEditorOptions } from '../../../browser/widget/chatOptions.js'; -import { IChatService } from '../../../common/chatService/chatService.js'; +import { ChatRequestQueueKind, IChatService } from '../../../common/chatService/chatService.js'; import { IChatSideChatService } from '../../../common/chatSideChatService.js'; import { ChatAgentLocation, ChatConfiguration, ChatModeKind } from '../../../common/constants.js'; -import { ChatModel } from '../../../common/model/chatModel.js'; +import { ChatModel, ChatRequestModel } from '../../../common/model/chatModel.js'; import { ChatToolInvocation } from '../../../common/model/chatProgressTypes/chatToolInvocation.js'; import { ChatViewModel, isRequestVM, isResponseVM } from '../../../common/model/chatViewModel.js'; import { ChatAgentService, IChatAgentService } from '../../../common/participants/chatAgents.js'; @@ -428,6 +428,58 @@ suite('ChatListWidget', () => { disposables.dispose(); }); + test('refreshes editing affordances when a queued request becomes steering', async () => { + const { disposables, model, widget } = createWidget({ + rendererOptions: { editable: true }, + }, configurationService => { + configurationService.setUserConfiguration(ChatConfiguration.EditRequests, 'inline'); + }); + const text = 'pending request'; + const request = new ChatRequestModel({ + session: model, + message: { + text, + parts: [new ChatRequestTextPart(new OffsetRange(0, text.length), new Range(1, 1, 1, text.length + 1), text)], + }, + variableData: { variables: [] }, + timestamp: 0, + }); + model.addPendingRequest(request, ChatRequestQueueKind.Queued, {}); + widget.refresh(); + widget.layout(300, 500); + await waitForStableLayout(widget); + + let editEvents = 0; + disposables.add(widget.onDidClickRequest(() => editEvents++)); + const states = []; + for (const kind of [ChatRequestQueueKind.Queued, ChatRequestQueueKind.Steering, ChatRequestQueueKind.Queued]) { + model.setPendingRequests([{ requestId: request.id, kind }]); + widget.refresh(); + const template = widget.getTemplateDataForRequestId(request.id); + assert.ok(template && isRequestVM(template.currentElement)); + const markdown = template.value.querySelector('.rendered-markdown'); + assert.ok(markdown); + editEvents = 0; + markdown.click(); + for (const keyCode of [13, 32]) { + markdown.dispatchEvent(new mainWindow.KeyboardEvent('keydown', { keyCode, bubbles: true, cancelable: true })); + } + states.push({ + pendingKind: template.currentElement.pendingKind, + clickable: markdown.classList.contains('clickable'), + editEvents, + }); + } + + assert.deepStrictEqual(states, [ + { pendingKind: ChatRequestQueueKind.Queued, clickable: true, editEvents: 3 }, + { pendingKind: ChatRequestQueueKind.Steering, clickable: false, editEvents: 0 }, + { pendingKind: ChatRequestQueueKind.Queued, clickable: true, editEvents: 3 }, + ]); + + disposables.dispose(); + }); + test('keeps tree sticky scroll disabled when the legacy prompt header is selected', () => { const { disposables, container } = createWidget({}, configurationService => { configurationService.setUserConfiguration(PROMPT_TIMELINE_STICKY_SCROLL_SETTING, true); From b3878b535347a247180e99058280c7dbac2b05d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joaqu=C3=ADn=20Ruales?= <1588988+jruales@users.noreply.github.com> Date: Sun, 20 Sep 2026 15:18:07 -0700 Subject: [PATCH 4/4] Revalidate queued edits before submission Check the live pending request state before preparing an edit and again before replacing the queued request. Reject stale edits with a warning while retaining the user's input. Refs #325884 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../contrib/chat/browser/widget/chatWidget.ts | 20 +++- .../test/browser/widget/chatWidget.test.ts | 110 +++++++++++++++--- 2 files changed, 115 insertions(+), 15 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts b/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts index 626d51b17a60e5..06eda31ccff351 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts @@ -44,6 +44,7 @@ import { ITextResourceEditorInput } from '../../../../../platform/editor/common/ import { IInstantiationService } from '../../../../../platform/instantiation/common/instantiation.js'; import { ServiceCollection } from '../../../../../platform/instantiation/common/serviceCollection.js'; import { ILogService } from '../../../../../platform/log/common/log.js'; +import { INotificationService } from '../../../../../platform/notification/common/notification.js'; import { bindContextKey } from '../../../../../platform/observable/common/platformObservableUtils.js'; import product from '../../../../../platform/product/common/product.js'; import { Progress } from '../../../../../platform/progress/common/progress.js'; @@ -584,6 +585,7 @@ export class ChatWidget extends Disposable implements IChatWidget { @IChatPasteTargetService private readonly chatPasteTargetService: IChatPasteTargetService, @IChatAccessibilityService private readonly chatAccessibilityService: IChatAccessibilityService, @ILogService private readonly logService: ILogService, + @INotificationService private readonly notificationService: INotificationService, @IThemeService private readonly themeService: IThemeService, @IChatSlashCommandService private readonly chatSlashCommandService: IChatSlashCommandService, @IChatEditingService chatEditingService: IChatEditingService, @@ -3240,6 +3242,19 @@ export class ChatWidget extends Disposable implements IChatWidget { return true; } + private _validateRequestEdit(): boolean { + const editing = this.viewModel?.editing; + if (!editing || editing.pendingKind === undefined) { + return true; + } + if (this.viewModel?.model.getPendingRequests().some(pending => pending.request.id === editing.id && pending.kind === ChatRequestQueueKind.Queued)) { + return true; + } + + this.notificationService.warn(localize('chat.editRequest.noLongerQueued', "This message is no longer queued and cannot be edited. Your edits have been kept in the input.")); + return false; + } + private async _acceptInput(query: { query: string } | undefined, options: IChatAcceptInputOptions = {}): Promise { if (!query && this.input.generating) { // if the user submits the input and generation finishes quickly, just submit it for them @@ -3255,7 +3270,7 @@ export class ChatWidget extends Disposable implements IChatWidget { await Event.toPromise(this.onDidChangeViewModel, this._store); } - if (!this.viewModel) { + if (!this.viewModel || !this._validateRequestEdit()) { return; } @@ -3320,6 +3335,9 @@ export class ChatWidget extends Disposable implements IChatWidget { if (await this._executeSlashCommandDuringRequest(requestInputs.input, { attachedContext }, isUserQuery, options.preserveFocus)) { return; } + if (!this._validateRequestEdit()) { + return; + } const isEditing = this.viewModel?.editing; const submittedFromEditing = shouldUnlockChatPetRequestRevision(isEditing !== undefined, isUserQuery); // Captured before `finishedEditing` tears the inline editor down, while `this.input` still diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/chatWidget.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/chatWidget.test.ts index 7fcab22ce4d514..0341739058e5de 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/chatWidget.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/chatWidget.test.ts @@ -8,13 +8,14 @@ import { mainWindow } from '../../../../../../base/browser/window.js'; import { DeferredPromise, timeout } from '../../../../../../base/common/async.js'; import { Emitter, Event } from '../../../../../../base/common/event.js'; import { Disposable, MutableDisposable } from '../../../../../../base/common/lifecycle.js'; -import { observableValue } from '../../../../../../base/common/observable.js'; +import { constObservable, observableValue } from '../../../../../../base/common/observable.js'; import { URI } from '../../../../../../base/common/uri.js'; import { mockObject, upcastPartial } from '../../../../../../base/test/common/mock.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js'; import { OffsetRange } from '../../../../../../editor/common/core/ranges/offsetRange.js'; import { Range } from '../../../../../../editor/common/core/range.js'; import { TestConfigurationService } from '../../../../../../platform/configuration/test/common/testConfigurationService.js'; +import { INotificationService } from '../../../../../../platform/notification/common/notification.js'; import { NullTelemetryService } from '../../../../../../platform/telemetry/common/telemetryUtils.js'; import { SaveReason } from '../../../../../common/editor.js'; import { ISaveAllEditorsOptions, ISaveEditorsResult } from '../../../../../services/editor/common/editorService.js'; @@ -22,9 +23,12 @@ import { TestEditorService } from '../../../../../test/browser/workbenchTestServ import { acceptAndAwaitSentRequest, ChatWidget, computeChatSessionStateIndicatorState, getImmediateSilentSlashCommandPart, layoutChatWidgetForInputHeight, saveAllBeforeChatSend, shouldShowChatTip, shouldShowChatWelcome, shouldUnlockChatPetQueueOrSteeringMessage, shouldUnlockChatPetRequestRevision } from '../../../browser/widget/chatWidget.js'; import { IChatListItemTemplate } from '../../../browser/widget/chatListRenderer.js'; import { IChatListItemRendererOptions } from '../../../browser/chat.js'; +import { IChatSubmitRequestHandlerService } from '../../../browser/chatSubmitRequestHandlerService.js'; import { ChatInputPart } from '../../../browser/widget/input/chatInputPart.js'; -import { ChatRequestQueueKind, ChatSendResult, ChatSendResultSent, IChatSendRequestData } from '../../../common/chatService/chatService.js'; -import { ChatAgentLocation, ChatConfiguration } from '../../../common/constants.js'; +import { ChatRequestVariableSet } from '../../../common/attachments/chatVariableEntries.js'; +import { ChatRequestQueueKind, ChatSendResult, ChatSendResultSent, IChatSendRequestData, IChatService } from '../../../common/chatService/chatService.js'; +import { ChatAgentLocation, ChatConfiguration, ChatModeKind } from '../../../common/constants.js'; +import { IChatPendingRequest, IChatRequestModel } from '../../../common/model/chatModel.js'; import { computeChatModelIsIdle } from '../../../common/model/chatModelIdle.js'; import { IChatRequestViewModel } from '../../../common/model/chatViewModel.js'; import { ChatRequestSlashCommandPart, ChatRequestTextPart, IParsedChatRequest } from '../../../common/requestParser/chatParserTypes.js'; @@ -156,21 +160,30 @@ suite('ChatWidget', () => { }]); }); - test('editing a queued request passes its model and configuration to the input', async () => { + async function createQueuedRequestEditWidget() { const modelId = 'agent-host-copilot:claude-opus-4.8'; const modelConfiguration = { reasoningEffort: 'xhigh' }; const configurationService = new TestConfigurationService(); await configurationService.setUserConfiguration('chat.editRequests', 'input'); + await configurationService.setUserConfiguration(ChatConfiguration.SaveBeforeSend, false); + let inputValue = 'original request'; const input = mockObject()({ element: mainWindow.document.createElement('div'), + currentModeKind: ChatModeKind.Agent, + generating: undefined, + hasPendingProgrammaticModelSelection: false, inputEditor: upcastPartial({ - getValue: () => 'original request', getModel: () => null, focus: () => { }, + getValue: () => inputValue, getModel: () => null, focus: () => { }, }), attachmentModel: upcastPartial({ getAttachmentIDs: () => new Set() }), dnd: upcastPartial({ setDisabledOverlay: () => { } }), onDidClickOverlay: Event.None, }); input.requestModelByIdentifier.resolves(true); + input.setValue.callsFake(value => { inputValue = value; }); + const attachedContext = new ChatRequestVariableSet(); + input.getAttachedContext.returns(attachedContext); + input.getAttachedAndImplicitContext.returns(attachedContext); const request = upcastPartial({ id: 'request', message: { text: 'original request', parts: [] }, @@ -180,21 +193,40 @@ suite('ChatWidget', () => { modelConfiguration, pendingKind: ChatRequestQueueKind.Queued, }); + const pendingRequest: IChatPendingRequest = { + request: upcastPartial({ id: request.id }), + kind: ChatRequestQueueKind.Queued, + sendOptions: {}, + }; + let pendingRequests: readonly IChatPendingRequest[] = [pendingRequest]; let editing: IChatRequestViewModel | undefined; + const viewModel = { + model: { + getRequests: () => [], + getPendingRequests: () => pendingRequests, + setCheckpoint: () => { }, + hasActiveRequest: constObservable(false), + }, + sessionResource: URI.parse('agent-host-copilot:/session'), + get editing() { return editing; }, + setEditing: (request: IChatRequestViewModel | undefined) => { editing = request; }, + }; + const chatService = mockObject()({}); + const notificationService = mockObject()({}); + const submitRequestHandlerService = mockObject()({}); + submitRequestHandlerService.tryHandle.resolves(false); const widget = Object.create(ChatWidget.prototype) as ChatWidget; Object.defineProperties(widget, { _store: { value: store }, _editingAutoScrollHold: { value: store.add(new MutableDisposable()) }, + _onDidAcceptInput: { value: store.add(new Emitter()) }, configurationService: { value: configurationService }, telemetryService: { value: NullTelemetryService }, - viewModel: { - value: { - model: { getRequests: () => [], setCheckpoint: () => { } }, - sessionResource: URI.parse('agent-host-copilot:/session'), - get editing() { return editing; }, - setEditing: (request: IChatRequestViewModel) => { editing = request; }, - }, - }, + chatService: { value: chatService }, + notificationService: { value: notificationService }, + chatSubmitRequestHandlerService: { value: submitRequestHandlerService }, + _viewModel: { value: viewModel }, + viewOptions: { value: {} }, input: { value: input }, inputPart: { value: input }, contribs: { value: [] }, @@ -203,15 +235,65 @@ suite('ChatWidget', () => { value: { getTemplateDataForRequestId: () => ({ currentElement: request }), acquireAutoScrollHold: () => Disposable.None, + setScrollLock: () => { }, }, }, }); + return { + widget, input, request, chatService, notificationService, submitRequestHandlerService, + setPendingKind: (kind: ChatRequestQueueKind | undefined) => { + pendingRequests = kind === undefined ? [] : [{ ...pendingRequest, kind }]; + }, + }; + } + + test('editing a queued request passes its model and configuration to the input', async () => { + const { widget, input, request } = await createQueuedRequestEditWidget(); widget.startEditing(request.id); - assert.deepStrictEqual(input.requestModelByIdentifier.firstCall.args, [modelId, modelConfiguration]); + assert.deepStrictEqual(input.requestModelByIdentifier.firstCall.args, [request.modelId, request.modelConfiguration]); }); + for (const duringSubmission of [false, true]) { + for (const kind of [ChatRequestQueueKind.Steering, undefined]) { + test(`preserves edits when the queued request is ${kind === undefined ? 'removed' : 'changed to steering'} ${duringSubmission ? 'during' : 'before'} submission`, async () => { + const { widget, input, request, chatService, notificationService, submitRequestHandlerService, setPendingKind } = await createQueuedRequestEditWidget(); + widget.startEditing(request.id); + input.setValue('edited request', false); + chatService.removePendingRequest.callsFake(() => assert.fail('Must not remove a request that is no longer queued')); + if (duringSubmission) { + submitRequestHandlerService.tryHandle.callsFake(async () => { + setPendingKind(kind); + return false; + }); + } else { + setPendingKind(kind); + } + + await widget.acceptInput(); + + assert.deepStrictEqual({ + pendingKinds: widget.viewModel?.model.getPendingRequests().map(pending => pending.kind), + editingRequest: widget.viewModel?.editing?.id, + input: widget.getInput(), + removed: chatService.removePendingRequest.callCount, + sent: chatService.sendRequest.callCount, + prepared: submitRequestHandlerService.tryHandle.callCount, + warnings: notificationService.warn.args, + }, { + pendingKinds: kind === undefined ? [] : [kind], + editingRequest: request.id, + input: 'edited request', + removed: 0, + sent: 0, + prepared: duringSubmission ? 1 : 0, + warnings: [['This message is no longer queued and cannot be edited. Your edits have been kept in the input.']], + }); + }); + } + } + test('does not start editing a pending steering request', () => { const request = upcastPartial({ id: 'steering-request',