From 6113b646d5f418993202e284d11ec0013703f9af Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Mon, 23 Oct 2023 16:26:45 -0700 Subject: [PATCH] Fix retrieving dynamic references for chat agents (#196320) * Fix retrieving dynamic references for chat agents Not a perfect fix to the underlying issue but solves the problem for now * Fix tests * Fix tests * Fix integration tests --- .../browser/contrib/chatDynamicReferences.ts | 4 +-- .../browser/contrib/chatInputEditorContrib.ts | 8 ++--- .../contrib/chat/browser/media/chat.css | 9 ++++-- .../contrib/chat/common/chatParserTypes.ts | 12 +++---- .../contrib/chat/common/chatRequestParser.ts | 25 +++++++-------- .../contrib/chat/common/chatServiceImpl.ts | 4 +-- .../chat/test/browser/chatVariables.test.ts | 3 +- .../chat/test/browser/mockChatWidget.ts | 31 +++++++++++++++++++ .../test/common/chatRequestParser.test.ts | 4 +++ .../chat/test/common/chatService.test.ts | 3 +- .../chat/test/common/mockChatVariables.ts | 2 +- 11 files changed, 71 insertions(+), 34 deletions(-) create mode 100644 src/vs/workbench/contrib/chat/test/browser/mockChatWidget.ts diff --git a/src/vs/workbench/contrib/chat/browser/contrib/chatDynamicReferences.ts b/src/vs/workbench/contrib/chat/browser/contrib/chatDynamicReferences.ts index cef2b2f14cb..1bbbb27ba9e 100644 --- a/src/vs/workbench/contrib/chat/browser/contrib/chatDynamicReferences.ts +++ b/src/vs/workbench/contrib/chat/browser/contrib/chatDynamicReferences.ts @@ -26,7 +26,7 @@ export class ChatDynamicReferenceModel extends Disposable implements IChatWidget private readonly _references: IDynamicReference[] = []; get references(): ReadonlyArray { - return this._references; + return [...this._references]; } get id() { @@ -126,7 +126,7 @@ export class SelectAndInsertFileAction extends Action2 { const fileName = basename(resource); const editor = context.widget.inputEditor; - const text = `$file:${fileName}`; + const text = `#file:${fileName}`; const range = context.range; const success = editor.executeEdits('chatInsertFile', [{ range, text: text + ' ' }]); if (!success) { diff --git a/src/vs/workbench/contrib/chat/browser/contrib/chatInputEditorContrib.ts b/src/vs/workbench/contrib/chat/browser/contrib/chatInputEditorContrib.ts index d14a5383e33..88c048b4268 100644 --- a/src/vs/workbench/contrib/chat/browser/contrib/chatInputEditorContrib.ts +++ b/src/vs/workbench/contrib/chat/browser/contrib/chatInputEditorContrib.ts @@ -501,7 +501,7 @@ class AgentCompletions extends Disposable { } class BuiltinDynamicCompletions extends Disposable { - private static readonly VariableNameDef = /\$\w*/g; // MUST be using `g`-flag + private static readonly VariableNameDef = new RegExp(`${chatVariableLeader}\\w*`, 'g'); // MUST be using `g`-flag constructor( @ILanguageFeaturesService private readonly languageFeaturesService: ILanguageFeaturesService, @@ -513,7 +513,7 @@ class BuiltinDynamicCompletions extends Disposable { this._register(this.languageFeaturesService.completionProvider.register({ scheme: ChatInputPart.INPUT_SCHEME, hasAccessToAllModels: true }, { _debugDisplayName: 'chatDynamicCompletions', - triggerCharacters: ['$'], + triggerCharacters: [chatVariableLeader], provideCompletionItems: async (model: ITextModel, position: Position, _context: CompletionContext, _token: CancellationToken) => { const fileVariablesEnabled = this.configurationService.getValue('chat.experimental.fileVariables') ?? this.productService.quality !== 'stable'; if (!fileVariablesEnabled) { @@ -544,8 +544,8 @@ class BuiltinDynamicCompletions extends Disposable { return { suggestions: [ { - label: '$file', - insertText: '$file:', + label: `${chatVariableLeader}file`, + insertText: `${chatVariableLeader}file:`, detail: localize('pickFileLabel', "Pick a file"), range: { insert, replace }, kind: CompletionItemKind.Text, diff --git a/src/vs/workbench/contrib/chat/browser/media/chat.css b/src/vs/workbench/contrib/chat/browser/media/chat.css index 771c5a91a10..ee3640e4aab 100644 --- a/src/vs/workbench/contrib/chat/browser/media/chat.css +++ b/src/vs/workbench/contrib/chat/browser/media/chat.css @@ -54,16 +54,19 @@ @keyframes ellipsis { 0% { + content: ""; + } + 25% { content: "."; } - 33% { + 50% { content: ".."; } - 66% { + 75% { content: "..."; } 100% { - content: "."; + content: ""; } } diff --git a/src/vs/workbench/contrib/chat/common/chatParserTypes.ts b/src/vs/workbench/contrib/chat/common/chatParserTypes.ts index a44a92d6bdf..0e230f81490 100644 --- a/src/vs/workbench/contrib/chat/common/chatParserTypes.ts +++ b/src/vs/workbench/contrib/chat/common/chatParserTypes.ts @@ -66,7 +66,7 @@ export class ChatRequestAgentPart implements IParsedChatRequestPart { constructor(readonly range: OffsetRange, readonly editorRange: IRange, readonly agent: IChatAgent) { } get text(): string { - return `@${this.agent.id}`; + return `${chatAgentLeader}${this.agent.id}`; } get promptText(): string { @@ -83,7 +83,7 @@ export class ChatRequestAgentSubcommandPart implements IParsedChatRequestPart { constructor(readonly range: OffsetRange, readonly editorRange: IRange, readonly command: IChatAgentCommand) { } get text(): string { - return `/${this.command.name}`; + return `${chatVariableLeader}${this.command.name}`; } get promptText(): string { @@ -100,16 +100,16 @@ export class ChatRequestSlashCommandPart implements IParsedChatRequestPart { constructor(readonly range: OffsetRange, readonly editorRange: IRange, readonly slashCommand: ISlashCommand) { } get text(): string { - return `/${this.slashCommand.command}`; + return `${chatSubcommandLeader}${this.slashCommand.command}`; } get promptText(): string { - return `/${this.slashCommand.command}`; + return `${chatSubcommandLeader}${this.slashCommand.command}`; } } /** - * An invocation of a dynamic reference like '$file:' + * An invocation of a dynamic reference like '#file:' */ export class ChatRequestDynamicReferencePart implements IParsedChatRequestPart { static readonly Kind = 'dynamic'; @@ -121,7 +121,7 @@ export class ChatRequestDynamicReferencePart implements IParsedChatRequestPart { } get text(): string { - return `$${this.referenceText}`; + return `${chatVariableLeader}${this.referenceText}`; } get promptText(): string { diff --git a/src/vs/workbench/contrib/chat/common/chatRequestParser.ts b/src/vs/workbench/contrib/chat/common/chatRequestParser.ts index 9491e40cf77..da606aac369 100644 --- a/src/vs/workbench/contrib/chat/common/chatRequestParser.ts +++ b/src/vs/workbench/contrib/chat/common/chatRequestParser.ts @@ -8,14 +8,14 @@ import { OffsetRange } from 'vs/editor/common/core/offsetRange'; import { IPosition, Position } from 'vs/editor/common/core/position'; import { Range } from 'vs/editor/common/core/range'; import { IChatAgentService } from 'vs/workbench/contrib/chat/common/chatAgents'; -import { ChatRequestAgentPart, ChatRequestAgentSubcommandPart, ChatRequestDynamicReferencePart, ChatRequestSlashCommandPart, ChatRequestTextPart, ChatRequestVariablePart, IParsedChatRequest, IParsedChatRequestPart, chatVariableLeader } from 'vs/workbench/contrib/chat/common/chatParserTypes'; +import { ChatRequestAgentPart, ChatRequestAgentSubcommandPart, ChatRequestDynamicReferencePart, ChatRequestSlashCommandPart, ChatRequestTextPart, ChatRequestVariablePart, IParsedChatRequest, IParsedChatRequestPart, chatAgentLeader, chatSubcommandLeader, chatVariableLeader } from 'vs/workbench/contrib/chat/common/chatParserTypes'; import { IChatService } from 'vs/workbench/contrib/chat/common/chatService'; -import { IChatVariablesService } from 'vs/workbench/contrib/chat/common/chatVariables'; +import { IChatVariablesService, IDynamicReference } from 'vs/workbench/contrib/chat/common/chatVariables'; const agentReg = /^@([\w_\-]+)(?=(\s|$|\b))/i; // An @-agent const variableReg = /^#([\w_\-]+)(:\d+)?(?=(\s|$|\b))/i; // A #-variable with an optional numeric : arg (@response:2) const slashReg = /\/([\w_\-]+)(?=(\s|$|\b))/i; // A / command -const dollarSignVarReg = /\$([\w_\-]+):([\w_\-\.]+)(?=(\s|$|\b))/i; // A / command +const variableWithArgReg = /\#([\w_\-]+):([\w_\-\.]+)(?=(\s|$|\b))/i; // A variable with a string : arg (#file:foo.ts) export class ChatRequestParser { constructor( @@ -26,6 +26,7 @@ export class ChatRequestParser { async parseChatRequest(sessionId: string, message: string): Promise { const parts: IParsedChatRequestPart[] = []; + const references = this.variableService.getDynamicReferences(sessionId); // must access this list before any async calls let lineNumber = 1; let column = 1; @@ -35,14 +36,13 @@ export class ChatRequestParser { let newPart: IParsedChatRequestPart | undefined; if (previousChar.match(/\s/) || i === 0) { if (char === chatVariableLeader) { - newPart = this.tryToParseVariable(message.slice(i), i, new Position(lineNumber, column), parts); - } else if (char === '@') { + newPart = this.tryToParseVariable(message.slice(i), i, new Position(lineNumber, column), parts) || + await this.tryToParseDynamicVariable(message.slice(i), i, new Position(lineNumber, column), references); + } else if (char === chatAgentLeader) { newPart = this.tryToParseAgent(message.slice(i), message, i, new Position(lineNumber, column), parts); - } else if (char === '/') { + } else if (char === chatSubcommandLeader) { // TODO try to make this sync newPart = await this.tryToParseSlashCommand(sessionId, message.slice(i), message, i, new Position(lineNumber, column), parts); - } else if (char === '$') { - newPart = await this.tryToParseDynamicVariable(sessionId, message.slice(i), i, new Position(lineNumber, column), parts); } } @@ -185,8 +185,8 @@ export class ChatRequestParser { return; } - private async tryToParseDynamicVariable(sessionId: string, message: string, offset: number, position: IPosition, parts: ReadonlyArray): Promise { - const nextVarMatch = message.match(dollarSignVarReg); + private async tryToParseDynamicVariable(message: string, offset: number, position: IPosition, references: ReadonlyArray): Promise { + const nextVarMatch = message.match(variableWithArgReg); if (!nextVarMatch) { return; } @@ -200,12 +200,9 @@ export class ChatRequestParser { return; } - const references = this.variableService.getDynamicReferences(sessionId); const refAtThisPosition = references.find(r => r.range.startLineNumber === position.lineNumber && - r.range.startColumn === position.column && - r.range.endLineNumber === position.lineNumber && - r.range.endColumn === position.column + full.length); + r.range.startColumn === position.column); if (refAtThisPosition) { return new ChatRequestDynamicReferencePart(range, editorRange, name, arg, refAtThisPosition.data); } diff --git a/src/vs/workbench/contrib/chat/common/chatServiceImpl.ts b/src/vs/workbench/contrib/chat/common/chatServiceImpl.ts index d91f6e485d7..d69ac7b7f5a 100644 --- a/src/vs/workbench/contrib/chat/common/chatServiceImpl.ts +++ b/src/vs/workbench/contrib/chat/common/chatServiceImpl.ts @@ -680,9 +680,9 @@ export class ChatService extends Disposable implements IChatService { } } - async sendRequestToProvider(sessionId: string, message: IChatDynamicRequest): Promise { + async sendRequestToProvider(sessionId: string, message: IChatDynamicRequest): Promise<{ responseCompletePromise: Promise } | undefined> { this.trace('sendRequestToProvider', `sessionId: ${sessionId}`); - await this.sendRequest(sessionId, message.message); + return await this.sendRequest(sessionId, message.message); } getProviders(): string[] { diff --git a/src/vs/workbench/contrib/chat/test/browser/chatVariables.test.ts b/src/vs/workbench/contrib/chat/test/browser/chatVariables.test.ts index 0aca445bedc..81517985bde 100644 --- a/src/vs/workbench/contrib/chat/test/browser/chatVariables.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/chatVariables.test.ts @@ -13,6 +13,7 @@ import { ChatVariablesService } from 'vs/workbench/contrib/chat/browser/chatVari import { ChatAgentService, IChatAgentService } from 'vs/workbench/contrib/chat/common/chatAgents'; import { ChatRequestParser } from 'vs/workbench/contrib/chat/common/chatRequestParser'; import { IChatVariablesService } from 'vs/workbench/contrib/chat/common/chatVariables'; +import { MockChatWidgetService } from 'vs/workbench/contrib/chat/test/browser/mockChatWidget'; import { IExtensionService } from 'vs/workbench/services/extensions/common/extensions'; import { TestExtensionService, TestStorageService } from 'vs/workbench/test/common/workbenchTestServices'; @@ -23,7 +24,7 @@ suite('ChatVariables', function () { const testDisposables = ensureNoDisposablesAreLeakedInTestSuite(); setup(function () { - service = new ChatVariablesService(null!); + service = new ChatVariablesService(new MockChatWidgetService()); instantiationService = testDisposables.add(new TestInstantiationService()); instantiationService.stub(IStorageService, testDisposables.add(new TestStorageService())); instantiationService.stub(ILogService, new NullLogService()); diff --git a/src/vs/workbench/contrib/chat/test/browser/mockChatWidget.ts b/src/vs/workbench/contrib/chat/test/browser/mockChatWidget.ts new file mode 100644 index 00000000000..7e23620a06e --- /dev/null +++ b/src/vs/workbench/contrib/chat/test/browser/mockChatWidget.ts @@ -0,0 +1,31 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { URI } from 'vs/base/common/uri'; +import { IChatWidget, IChatWidgetService } from 'vs/workbench/contrib/chat/browser/chat'; + +export class MockChatWidgetService implements IChatWidgetService { + readonly _serviceBrand: undefined; + + /** + * Returns the most recently focused widget if any. + */ + readonly lastFocusedWidget: IChatWidget | undefined; + + /** + * Returns whether a view was successfully revealed. + */ + async revealViewForProvider(providerId: string): Promise { + return undefined; + } + + getWidgetByInputUri(uri: URI): IChatWidget | undefined { + return undefined; + } + + getWidgetBySessionId(sessionId: string): IChatWidget | undefined { + return undefined; + } +} diff --git a/src/vs/workbench/contrib/chat/test/common/chatRequestParser.test.ts b/src/vs/workbench/contrib/chat/test/common/chatRequestParser.test.ts index f3e56c5ff02..7619ef46437 100644 --- a/src/vs/workbench/contrib/chat/test/common/chatRequestParser.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/chatRequestParser.test.ts @@ -28,6 +28,10 @@ suite('ChatRequestParser', () => { instantiationService.stub(ILogService, new NullLogService()); instantiationService.stub(IExtensionService, new TestExtensionService()); instantiationService.stub(IChatAgentService, testDisposables.add(instantiationService.createInstance(ChatAgentService))); + + const varService = mockObject()({}); + varService.getDynamicReferences.returns([]); + instantiationService.stub(IChatVariablesService, varService as any); }); test('plain text', async () => { diff --git a/src/vs/workbench/contrib/chat/test/common/chatService.test.ts b/src/vs/workbench/contrib/chat/test/common/chatService.test.ts index 4e7f0044a4f..eebf8589487 100644 --- a/src/vs/workbench/contrib/chat/test/common/chatService.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/chatService.test.ts @@ -230,7 +230,8 @@ suite('Chat', () => { const model = testDisposables.add(testService.startSession('testProvider', CancellationToken.None)); assert.strictEqual(model.getRequests().length, 0); - await testService.sendRequestToProvider(model.sessionId, { message: 'test request' }); + const response = await testService.sendRequestToProvider(model.sessionId, { message: 'test request' }); + await response?.responseCompletePromise; assert.strictEqual(model.getRequests().length, 1); }); diff --git a/src/vs/workbench/contrib/chat/test/common/mockChatVariables.ts b/src/vs/workbench/contrib/chat/test/common/mockChatVariables.ts index 8f688bbd069..b0750ae8d41 100644 --- a/src/vs/workbench/contrib/chat/test/common/mockChatVariables.ts +++ b/src/vs/workbench/contrib/chat/test/common/mockChatVariables.ts @@ -24,7 +24,7 @@ export class MockChatVariablesService implements IChatVariablesService { } getDynamicReferences(sessionId: string): readonly IDynamicReference[] { - throw new Error('Method not implemented.'); + return []; } async resolveVariables(prompt: IParsedChatRequest, model: IChatModel, token: CancellationToken): Promise {