From 04eff405cf5d3c108852d593a89b894545ca795f Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Fri, 15 Nov 2024 18:48:35 +0100 Subject: [PATCH] restore history sharing rules for agents (#233930) --- .../src/singlefolder-tests/chat.test.ts | 6 +++--- .../workbench/api/common/extHostChatAgents2.ts | 11 +++-------- .../contrib/chat/common/chatServiceImpl.ts | 15 ++------------- .../chat/test/common/chatService.test.ts | 18 ++++-------------- 4 files changed, 12 insertions(+), 38 deletions(-) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/chat.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/chat.test.ts index 6518dd519a5..ce1e544e505 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/chat.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/chat.test.ts @@ -120,7 +120,7 @@ suite('chat', () => { assert.deepStrictEqual(result.metadata, { key: 'value' }); }); - test('participant history not isolated within extension', async () => { + test('isolated participant history', async () => { const onRequest = setupParticipant(); const onRequest2 = setupParticipant(true); @@ -132,13 +132,13 @@ suite('chat', () => { commands.executeCommand('workbench.action.chat.open', { query: '@participant2 hi' }); }, 0); const request2 = await asPromise(onRequest2); - assert.strictEqual(request2.context.history.length, 10); + assert.strictEqual(request2.context.history.length, 0); setTimeout(() => { commands.executeCommand('workbench.action.chat.open', { query: '@participant2 hi' }); }, 0); const request3 = await asPromise(onRequest2); - assert.strictEqual(request3.context.history.length, 12); // request + response = 2 + assert.strictEqual(request3.context.history.length, 2); // request + response = 2 }); test('title provider is called for first request', async () => { diff --git a/src/vs/workbench/api/common/extHostChatAgents2.ts b/src/vs/workbench/api/common/extHostChatAgents2.ts index d581fe4081f..14d09843895 100644 --- a/src/vs/workbench/api/common/extHostChatAgents2.ts +++ b/src/vs/workbench/api/common/extHostChatAgents2.ts @@ -515,10 +515,9 @@ export class ExtHostChatAgents2 extends Disposable implements ExtHostChatAgentsS for (const h of context.history) { const ehResult = typeConvert.ChatAgentResult.to(h.result); - // get full result when agent is the same or agent is from the same extension - const result: vscode.ChatResult = agentId === h.request.agentId || ExtensionIdentifier.equals(this.getChatAgentById(agentId)?.extension.identifier, this.getChatAgentById(h.request.agentId)?.extension.identifier) - ? ehResult - : { ...ehResult, metadata: undefined }; + const result: vscode.ChatResult = agentId === h.request.agentId ? + ehResult : + { ...ehResult, metadata: undefined }; // REQUEST turn const varsWithoutTools = h.request.variables.variables @@ -538,10 +537,6 @@ export class ExtHostChatAgents2 extends Disposable implements ExtHostChatAgentsS return res; } - private getChatAgentById(id: string) { - return Iterable.find(this._agents.values(), a => a.id === id); - } - $releaseSession(sessionId: string): void { this._sessionDisposables.deleteAndDispose(sessionId); } diff --git a/src/vs/workbench/contrib/chat/common/chatServiceImpl.ts b/src/vs/workbench/contrib/chat/common/chatServiceImpl.ts index 26f5332430b..0d9c507a0c7 100644 --- a/src/vs/workbench/contrib/chat/common/chatServiceImpl.ts +++ b/src/vs/workbench/contrib/chat/common/chatServiceImpl.ts @@ -16,7 +16,6 @@ import { StopWatch } from '../../../../base/common/stopwatch.js'; import { URI, UriComponents } from '../../../../base/common/uri.js'; import { localize } from '../../../../nls.js'; import { IConfigurationService } from '../../../../platform/configuration/common/configuration.js'; -import { ExtensionIdentifier } from '../../../../platform/extensions/common/extensions.js'; import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js'; import { ILogService } from '../../../../platform/log/common/log.js'; import { Progress } from '../../../../platform/progress/common/progress.js'; @@ -785,23 +784,13 @@ export class ChatService extends Disposable implements IChatService { private getHistoryEntriesFromModel(requests: IChatRequestModel[], sessionId: string, location: ChatAgentLocation, forAgentId: string): IChatAgentHistoryEntry[] { const history: IChatAgentHistoryEntry[] = []; - - - const defaultAgentId = this.chatAgentService.getDefaultAgent(location)?.id; - - const forAgentExtension = this.chatAgentService.getAgent(forAgentId); - for (const request of requests) { if (!request.response) { continue; } - const responseAgentExtension = request.response.agent ? this.chatAgentService.getAgent(request.response.agent.id) : undefined; - - if (forAgentId !== defaultAgentId - && forAgentId !== request.response.agent?.id - && !ExtensionIdentifier.equals(forAgentExtension?.extensionId, responseAgentExtension?.extensionId) - ) { + const defaultAgentId = this.chatAgentService.getDefaultAgent(location)?.id; + if (forAgentId !== request.response.agent?.id && forAgentId !== defaultAgentId) { // An agent only gets to see requests that were sent to this agent. // The default agent (the undefined case) gets to see all of them. continue; 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 c96a79b08c6..393c3813b83 100644 --- a/src/vs/workbench/contrib/chat/test/common/chatService.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/chatService.test.ts @@ -34,7 +34,6 @@ import { IExtensionService, nullExtensionDescription } from '../../../../service import { IViewsService } from '../../../../services/views/common/viewsService.js'; import { TestContextService, TestExtensionService, TestStorageService } from '../../../../test/common/workbenchTestServices.js'; import { MarkdownString } from '../../../../../base/common/htmlContent.js'; -import { ExtensionIdentifier } from '../../../../../platform/extensions/common/extensions.js'; const chatAgentWithUsedContextId = 'ChatProviderWithUsedContext'; const chatAgentWithUsedContext: IChatAgent = { @@ -197,11 +196,9 @@ suite('ChatService', () => { }; testDisposables.add(chatAgentService.registerAgent('defaultAgent', { ...getAgentData('defaultAgent'), isDefault: true })); - testDisposables.add(chatAgentService.registerAgent('agent2', { ...getAgentData('agent2'), extensionId: new ExtensionIdentifier('foo.bar') })); - testDisposables.add(chatAgentService.registerAgent('agent3', getAgentData('agent3'))); + testDisposables.add(chatAgentService.registerAgent('agent2', getAgentData('agent2'))); testDisposables.add(chatAgentService.registerAgentImplementation('defaultAgent', historyLengthAgent)); testDisposables.add(chatAgentService.registerAgentImplementation('agent2', historyLengthAgent)); - testDisposables.add(chatAgentService.registerAgentImplementation('agent3', historyLengthAgent)); const testService = testDisposables.add(instantiationService.createInstance(ChatService)); const model = testDisposables.add(testService.startSession(ChatAgentLocation.Panel, CancellationToken.None)); @@ -220,19 +217,12 @@ suite('ChatService', () => { assert.strictEqual(model.getRequests().length, 2); assert.strictEqual(model.getRequests()[1].response?.result?.metadata?.historyLength, 0); - // Send a request to agent3- it can see the default agent's message because they are from the same extension - const response3 = await testService.sendRequest(model.sessionId, `test request`, { agentId: 'agent3' }); + // Send a request to defaultAgent - the default agent can see agent2's message + const response3 = await testService.sendRequest(model.sessionId, `test request`, { agentId: 'defaultAgent' }); assert(response3); await response3.responseCompletePromise; assert.strictEqual(model.getRequests().length, 3); - assert.strictEqual(model.getRequests()[2].response?.result?.metadata?.historyLength, 1); - - // Send a request to defaultAgent - the default agent can see agent2's message - const response4 = await testService.sendRequest(model.sessionId, `test request`, { agentId: 'defaultAgent' }); - assert(response4); - await response4.responseCompletePromise; - assert.strictEqual(model.getRequests().length, 4); - assert.strictEqual(model.getRequests()[3].response?.result?.metadata?.historyLength, 3); + assert.strictEqual(model.getRequests()[2].response?.result?.metadata?.historyLength, 2); }); test('can serialize', async () => {