From 0bcc2769facf5e6e24ff554d4650f66f45f5f55d Mon Sep 17 00:00:00 2001 From: Teresa Hoang <125500434+teresaqhoang@users.noreply.github.com> Date: Wed, 9 Aug 2023 09:09:17 -0700 Subject: [PATCH 1/5] =?UTF-8?q?=F0=9F=90=9B=20Fix=20connection=20error=20h?= =?UTF-8?q?andling=20+=20uncaught=20exceptions=20(#134)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Motivation and Context Fix connection errors in useChat and appSlice + fix red screens of death ### Description - Added an ID to Alerts in AppState, add logic to update connection status in appSlice, and update SignalRMiddleware to add an ID to connection alerts. - Added more error handling to reduce red screens of death - Move all getAccessToken calls into try catch blocks - Added some missing details around Stepwise Planner ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [x] The PR follows the [Contribution Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts) raises no violations ~~- [ ] All unit tests pass, and I have added new tests where possible~~ - [x] I didn't break anyone :smile: --- webapi/Options/PlannerOptions.cs | 2 +- webapi/appsettings.json | 1 + webapp/src/Constants.ts | 1 + webapp/src/checkEnv.ts | 5 +- .../token-usage/TokenUsageGraph.tsx | 3 +- webapp/src/components/views/BackendProbe.tsx | 4 +- webapp/src/libs/hooks/useChat.ts | 34 +++++++------- webapp/src/redux/features/app/AppState.ts | 1 + webapp/src/redux/features/app/appSlice.ts | 46 +++++++++++++++++-- .../message-relay/signalRMiddleware.ts | 21 +++++++-- 10 files changed, 91 insertions(+), 27 deletions(-) diff --git a/webapi/Options/PlannerOptions.cs b/webapi/Options/PlannerOptions.cs index 291533159..7d9c79d3f 100644 --- a/webapi/Options/PlannerOptions.cs +++ b/webapi/Options/PlannerOptions.cs @@ -33,7 +33,7 @@ public class MissingFunctionErrorOptions public const string PropertyName = "Planner"; /// - /// Define if the planner must be Sequential or not. + /// The type of planner to used to create plan. /// [Required] public PlanType Type { get; set; } = PlanType.Action; diff --git a/webapi/appsettings.json b/webapi/appsettings.json index d3a27c729..f7d7790f4 100644 --- a/webapi/appsettings.json +++ b/webapi/appsettings.json @@ -52,6 +52,7 @@ // - Set Planner:Type to "Action" to use the single-step ActionPlanner // - Set Planner:Type to "Sequential" to enable the multi-step SequentialPlanner // Note: SequentialPlanner works best with `gpt-4`. See the "Enabling Sequential Planner" section in webapi/README.md for configuration instructions. + // - Set Planner:Type to "Stepwise" to enable MRKL style planning // - Set Planner:RelevancyThreshold to a decimal between 0 and 1.0. // "Planner": { diff --git a/webapp/src/Constants.ts b/webapp/src/Constants.ts index 136499450..73db5cfde 100644 --- a/webapp/src/Constants.ts +++ b/webapp/src/Constants.ts @@ -4,6 +4,7 @@ export const Constants = { app: { name: 'Copilot', updateCheckIntervalSeconds: 60 * 5, + CONNECTION_ALERT_ID: 'connection-alert', }, msal: { method: 'redirect', // 'redirect' | 'popup' diff --git a/webapp/src/checkEnv.ts b/webapp/src/checkEnv.ts index afa5a8afd..f3b322c3a 100644 --- a/webapp/src/checkEnv.ts +++ b/webapp/src/checkEnv.ts @@ -1,7 +1,10 @@ +/** + * Checks if all required environment variables are defined + * @returns {string[]} An array of missing environment variables + */ export const getMissingEnvVariables = () => { // Should be aligned with variables defined in .env.example const envVariables = ['REACT_APP_BACKEND_URI', 'REACT_APP_AAD_AUTHORITY', 'REACT_APP_AAD_CLIENT_ID']; - const missingVariables = []; for (const variable of envVariables) { diff --git a/webapp/src/components/token-usage/TokenUsageGraph.tsx b/webapp/src/components/token-usage/TokenUsageGraph.tsx index 697ab01be..4ba0529cb 100644 --- a/webapp/src/components/token-usage/TokenUsageGraph.tsx +++ b/webapp/src/components/token-usage/TokenUsageGraph.tsx @@ -63,7 +63,8 @@ const contrastColors = [ export const TokenUsageGraph: React.FC = ({ promptView, tokenUsage }) => { const classes = useClasses(); const { conversations, selectedId } = useAppSelector((state: RootState) => state.conversations); - const loadingResponse = conversations[selectedId].botResponseStatus && Object.entries(tokenUsage).length === 0; + const loadingResponse = + selectedId !== '' && conversations[selectedId].botResponseStatus && Object.entries(tokenUsage).length === 0; const responseGenerationView: TokenUsageView = {}; const memoryGenerationView: TokenUsageView = {}; diff --git a/webapp/src/components/views/BackendProbe.tsx b/webapp/src/components/views/BackendProbe.tsx index a4eec1239..266963b86 100644 --- a/webapp/src/components/views/BackendProbe.tsx +++ b/webapp/src/components/views/BackendProbe.tsx @@ -20,7 +20,9 @@ const BackendProbe: FC = ({ uri, onBackendFound }) => { } }; - void fetchAsync(); + fetchAsync().catch(() => { + // Ignore - this page is just a probe, so we don't need to show any errors if backend is not found + }); }, 3000); return () => { diff --git a/webapp/src/libs/hooks/useChat.ts b/webapp/src/libs/hooks/useChat.ts index e4751107f..095a5b2f2 100644 --- a/webapp/src/libs/hooks/useChat.ts +++ b/webapp/src/libs/hooks/useChat.ts @@ -72,10 +72,9 @@ export const useChat = () => { const createChat = async () => { const chatTitle = `Copilot @ ${new Date().toLocaleString()}`; - const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress); try { await chatService - .createChatAsync(userId, chatTitle, accessToken) + .createChatAsync(userId, chatTitle, await AuthHelper.getSKaaSAccessToken(instance, inProgress)) .then((result: ICreateChatSessionResponse) => { const newChat: ChatState = { id: result.chatSession.id, @@ -148,8 +147,8 @@ export const useChat = () => { }; const loadChats = async () => { - const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress); try { + const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress); const chatSessions = await chatService.getAllChatsAsync(userId, accessToken); if (chatSessions.length > 0) { @@ -201,10 +200,9 @@ export const useChat = () => { }; const uploadBot = async (bot: Bot) => { - const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress); - botService - .uploadAsync(bot, userId, accessToken) - .then(async (chatSession: IChatSession) => { + try { + const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress); + await botService.uploadAsync(bot, userId, accessToken).then(async (chatSession: IChatSession) => { const chatMessages = await chatService.getChatMessagesAsync(chatSession.id, 0, 100, accessToken); const newChat = { @@ -217,11 +215,11 @@ export const useChat = () => { }; dispatch(addConversation(newChat)); - }) - .catch((e: any) => { - const errorMessage = `Unable to upload the bot. Details: ${getErrorDetails(e)}`; - dispatch(addAlert({ message: errorMessage, type: AlertType.Error })); }); + } catch (e: any) { + const errorMessage = `Unable to upload the bot. Details: ${getErrorDetails(e)}`; + dispatch(addAlert({ message: errorMessage, type: AlertType.Error })); + } }; const getBotProfilePicture = (index: number): string => { @@ -282,8 +280,8 @@ export const useChat = () => { }; const joinChat = async (chatId: string) => { - const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress); try { + const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress); await chatService.joinChatAsync(userId, chatId, accessToken).then(async (result: IChatSession) => { // Get chat messages const chatMessages = await chatService.getChatMessagesAsync(result.id, 0, 100, accessToken); @@ -315,9 +313,14 @@ export const useChat = () => { }; const editChat = async (chatId: string, title: string, syetemDescription: string, memoryBalance: number) => { - const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress); try { - await chatService.editChatAsync(chatId, title, syetemDescription, memoryBalance, accessToken); + await chatService.editChatAsync( + chatId, + title, + syetemDescription, + memoryBalance, + await AuthHelper.getSKaaSAccessToken(instance, inProgress), + ); } catch (e: any) { const errorMessage = `Error editing chat ${chatId}. Details: ${getErrorDetails(e)}`; dispatch(addAlert({ message: errorMessage, type: AlertType.Error })); @@ -325,9 +328,8 @@ export const useChat = () => { }; const getServiceOptions = async () => { - const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress); try { - return await chatService.getServiceOptionsAsync(accessToken); + return await chatService.getServiceOptionsAsync(await AuthHelper.getSKaaSAccessToken(instance, inProgress)); } catch (e: any) { const errorMessage = `Error getting service options. Details: ${getErrorDetails(e)}`; dispatch(addAlert({ message: errorMessage, type: AlertType.Error })); diff --git a/webapp/src/redux/features/app/AppState.ts b/webapp/src/redux/features/app/AppState.ts index a1664f69e..11cd7ecb0 100644 --- a/webapp/src/redux/features/app/AppState.ts +++ b/webapp/src/redux/features/app/AppState.ts @@ -13,6 +13,7 @@ export interface ActiveUserInfo { export interface Alert { message: string; type: AlertType; + id?: string; } interface Feature { diff --git a/webapp/src/redux/features/app/appSlice.ts b/webapp/src/redux/features/app/appSlice.ts index ff95e23b3..1d4ef00f2 100644 --- a/webapp/src/redux/features/app/appSlice.ts +++ b/webapp/src/redux/features/app/appSlice.ts @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. import { createSlice, PayloadAction } from '@reduxjs/toolkit'; +import { Constants } from '../../../Constants'; import { ServiceOptions } from '../../../libs/models/ServiceOptions'; import { TokenUsage } from '../../../libs/models/TokenUsage'; import { ActiveUserInfo, Alert, AppState, FeatureKeys, initialState } from './AppState'; @@ -13,10 +14,14 @@ export const appSlice = createSlice({ state.alerts = action.payload; }, addAlert: (state: AppState, action: PayloadAction) => { - if (state.alerts.length === 3) { - state.alerts.shift(); + if ( + action.payload.id == Constants.app.CONNECTION_ALERT_ID || + isServerConnectionError(action.payload.message) + ) { + updateConnectionStatus(state, action.payload); + } else { + addNewAlert(state.alerts, action.payload); } - state.alerts.push(action.payload); }, removeAlert: (state: AppState, action: PayloadAction) => { state.alerts.splice(action.payload, 1); @@ -89,3 +94,38 @@ const getTotalTokenUsage = (previousSum?: number, current?: number) => { return previousSum + current; }; + +const isServerConnectionError = (message: string) => { + return ( + message.includes(`Cannot send data if the connection is not in the 'Connected' State.`) || + message.includes(`Server timeout elapsed without receiving a message from the server.`) + ); +}; + +const addNewAlert = (alerts: Alert[], newAlert: Alert) => { + if (alerts.length === 3) { + alerts.shift(); + } + + alerts.push(newAlert); +}; + +const updateConnectionStatus = (state: AppState, statusUpdate: Alert) => { + if (isServerConnectionError(statusUpdate.message)) { + statusUpdate.message = + // Constant message so alert UI doesn't feel glitchy on every connection error from SignalR + 'Cannot send data due to lost connection or server timeout. Try refreshing this page to restart the connection.'; + } + + // There should only ever be one connection alert at a time, + // so we tag the alert with a unique ID so we can remove if needed + statusUpdate.id ??= Constants.app.CONNECTION_ALERT_ID; + + // Remove the existing connection alert if it exists + const connectionAlertIndex = state.alerts.findIndex((alert) => alert.id === Constants.app.CONNECTION_ALERT_ID); + if (connectionAlertIndex !== -1) { + state.alerts.splice(connectionAlertIndex, 1); + } + + addNewAlert(state.alerts, statusUpdate); +}; diff --git a/webapp/src/redux/features/message-relay/signalRMiddleware.ts b/webapp/src/redux/features/message-relay/signalRMiddleware.ts index 8ffc237ef..6beaac3fd 100644 --- a/webapp/src/redux/features/message-relay/signalRMiddleware.ts +++ b/webapp/src/redux/features/message-relay/signalRMiddleware.ts @@ -2,6 +2,7 @@ import * as signalR from '@microsoft/signalr'; import { AnyAction, Dispatch } from '@reduxjs/toolkit'; +import { Constants } from '../../../Constants'; import { AlertType } from '../../../libs/models/AlertType'; import { IChatUser } from '../../../libs/models/ChatUser'; import { PlanState } from '../../../libs/models/Plan'; @@ -65,7 +66,13 @@ const registerCommonSignalConnectionEvents = (store: Store) => { hubConnection.onclose((error) => { if (hubConnection.state === signalR.HubConnectionState.Disconnected) { const errorMessage = 'Connection closed due to error. Try refreshing this page to restart the connection'; - store.dispatch(addAlert({ message: String(errorMessage), type: AlertType.Error })); + store.dispatch( + addAlert({ + message: String(errorMessage), + type: AlertType.Error, + id: Constants.app.CONNECTION_ALERT_ID, + }), + ); console.log(errorMessage, error); } }); @@ -73,15 +80,21 @@ const registerCommonSignalConnectionEvents = (store: Store) => { hubConnection.onreconnecting((error) => { if (hubConnection.state === signalR.HubConnectionState.Reconnecting) { const errorMessage = 'Connection lost due to error. Reconnecting...'; - store.dispatch(addAlert({ message: String(errorMessage), type: AlertType.Info })); + store.dispatch( + addAlert({ + message: String(errorMessage), + type: AlertType.Info, + id: Constants.app.CONNECTION_ALERT_ID, + }), + ); console.log(errorMessage, error); } }); hubConnection.onreconnected((connectionId = '') => { if (hubConnection.state === signalR.HubConnectionState.Connected) { - const message = 'Connection reestablished.'; - store.dispatch(addAlert({ message, type: AlertType.Success })); + const message = 'Connection reestablished. Please refresh the page to ensure you have the latest data.'; + store.dispatch(addAlert({ message, type: AlertType.Success, id: Constants.app.CONNECTION_ALERT_ID })); console.log(message + ` Connected with connectionId ${connectionId}`); } }); From 1ed63be54cdf4cc2d347ee301054b7b3019c008a Mon Sep 17 00:00:00 2001 From: Chris <66376200+crickman@users.noreply.github.com> Date: Wed, 9 Aug 2023 12:57:54 -0700 Subject: [PATCH 2/5] Fix app registration link in README (#142) ### Motivation and Context The link to the application registration section has become zombied. ### Description Re-targeted existing reference. ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [x] The PR follows the [Contribution Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone :smile: --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 3a5d88d2c..bb02be1d1 100644 --- a/README.md +++ b/README.md @@ -141,7 +141,8 @@ You will need the following items to run the sample: ## (Optional) Enable backend authorization via Azure AD -1. Ensure you created the required application registration mentioned in [Start the WebApp FrontEnd application](#start-the-webapp-frontend-application) +1. Ensure you created the required application registration mentioned in [Register an application](#register-an-application) + 2. Create a second application registration to represent the web api > For more details on creating an application registration, go [here](https://learn.microsoft.com/en-us/azure/active-directory/develop/quickstart-register-app). From 6b6f369e7f70d974fde669fafad76ffba06858c0 Mon Sep 17 00:00:00 2001 From: Gil LaHaye Date: Wed, 9 Aug 2023 15:24:00 -0600 Subject: [PATCH 3/5] Rename "MemoriesStore" to "MemoryStore" (#138) ### Motivation and Context Just making this grammatically correct in English. (It was driving me nuts) ### Description Rename "MemoriesStore" to "MemoryStore" ### Contribution Checklist - [ ] The code builds clean without any errors or warnings - [ ] The PR follows the [Contribution Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [ ] All unit tests pass, and I have added new tests where possible - [ ] I didn't break anyone :smile: --- scripts/deploy/main.bicep | 12 ++-- scripts/deploy/main.json | 12 ++-- .../Controllers/ServiceOptionsController.cs | 12 ++-- webapi/Extensions/SemanticKernelExtensions.cs | 20 +++---- webapi/Extensions/ServiceExtensions.cs | 4 +- .../Models/Response/ServiceOptionsResponse.cs | 14 ++--- webapi/Options/MemoriesStoreOptions.cs | 60 ------------------- webapi/Options/MemoryStoreOptions.cs | 60 +++++++++++++++++++ .../Skills/ChatSkills/CopilotChatPlanner.cs | 2 +- webapi/appsettings.json | 10 ++-- .../src/components/chat/tabs/DocumentsTab.tsx | 6 +- webapp/src/libs/models/ServiceOptions.ts | 4 +- webapp/src/redux/features/app/AppState.ts | 2 +- 13 files changed, 109 insertions(+), 109 deletions(-) delete mode 100644 webapi/Options/MemoriesStoreOptions.cs create mode 100644 webapi/Options/MemoryStoreOptions.cs diff --git a/scripts/deploy/main.bicep b/scripts/deploy/main.bicep index 2434d58e7..569ebc312 100644 --- a/scripts/deploy/main.bicep +++ b/scripts/deploy/main.bicep @@ -227,27 +227,27 @@ resource appServiceWebConfig 'Microsoft.Web/sites/config@2022-09-01' = { value: deployCosmosDB ? cosmosAccount.listConnectionStrings().connectionStrings[0].connectionString : '' } { - name: 'MemoriesStore:Type' + name: 'MemoryStore:Type' value: memoryStore } { - name: 'MemoriesStore:Qdrant:Host' + name: 'MemoryStore:Qdrant:Host' value: memoryStore == 'Qdrant' ? 'https://${appServiceQdrant.properties.defaultHostName}' : '' } { - name: 'MemoriesStore:Qdrant:Port' + name: 'MemoryStore:Qdrant:Port' value: '443' } { - name: 'MemoriesStore:AzureCognitiveSearch:UseVectorSearch' + name: 'MemoryStore:AzureCognitiveSearch:UseVectorSearch' value: 'true' } { - name: 'MemoriesStore:AzureCognitiveSearch:Endpoint' + name: 'MemoryStore:AzureCognitiveSearch:Endpoint' value: memoryStore == 'AzureCognitiveSearch' ? 'https://${azureCognitiveSearch.name}.search.windows.net' : '' } { - name: 'MemoriesStore:AzureCognitiveSearch:Key' + name: 'MemoryStore:AzureCognitiveSearch:Key' value: memoryStore == 'AzureCognitiveSearch' ? azureCognitiveSearch.listAdminKeys().primaryKey : '' } { diff --git a/scripts/deploy/main.json b/scripts/deploy/main.json index 21a0c157d..8687527fd 100644 --- a/scripts/deploy/main.json +++ b/scripts/deploy/main.json @@ -336,27 +336,27 @@ "value": "[if(parameters('deployCosmosDB'), listConnectionStrings(resourceId('Microsoft.DocumentDB/databaseAccounts', toLower(format('cosmos-{0}', variables('uniqueName')))), '2023-04-15').connectionStrings[0].connectionString, '')]" }, { - "name": "MemoriesStore:Type", + "name": "MemoryStore:Type", "value": "[parameters('memoryStore')]" }, { - "name": "MemoriesStore:Qdrant:Host", + "name": "MemoryStore:Qdrant:Host", "value": "[if(equals(parameters('memoryStore'), 'Qdrant'), format('https://{0}', reference(resourceId('Microsoft.Web/sites', format('app-{0}-qdrant', variables('uniqueName'))), '2022-09-01').defaultHostName), '')]" }, { - "name": "MemoriesStore:Qdrant:Port", + "name": "MemoryStore:Qdrant:Port", "value": "443" }, { - "name": "MemoriesStore:AzureCognitiveSearch:UseVectorSearch", + "name": "MemoryStore:AzureCognitiveSearch:UseVectorSearch", "value": "true" }, { - "name": "MemoriesStore:AzureCognitiveSearch:Endpoint", + "name": "MemoryStore:AzureCognitiveSearch:Endpoint", "value": "[if(equals(parameters('memoryStore'), 'AzureCognitiveSearch'), format('https://{0}.search.windows.net', format('acs-{0}', variables('uniqueName'))), '')]" }, { - "name": "MemoriesStore:AzureCognitiveSearch:Key", + "name": "MemoryStore:AzureCognitiveSearch:Key", "value": "[if(equals(parameters('memoryStore'), 'AzureCognitiveSearch'), listAdminKeys(resourceId('Microsoft.Search/searchServices', format('acs-{0}', variables('uniqueName'))), '2022-09-01').primaryKey, '')]" }, { diff --git a/webapi/Controllers/ServiceOptionsController.cs b/webapi/Controllers/ServiceOptionsController.cs index 7a76f5219..28adf3758 100644 --- a/webapi/Controllers/ServiceOptionsController.cs +++ b/webapi/Controllers/ServiceOptionsController.cs @@ -20,14 +20,14 @@ public class ServiceOptionsController : ControllerBase { private readonly ILogger _logger; - private readonly MemoriesStoreOptions _memoriesStoreOptions; + private readonly MemoryStoreOptions _memoryStoreOptions; public ServiceOptionsController( ILogger logger, - IOptions memoriesStoreOptions) + IOptions memoryStoreOptions) { this._logger = logger; - this._memoriesStoreOptions = memoriesStoreOptions.Value; + this._memoryStoreOptions = memoryStoreOptions.Value; } // TODO: [Issue #95] Include all service options in a single response. @@ -42,10 +42,10 @@ public IActionResult GetServiceOptions() return this.Ok( new ServiceOptionsResponse() { - MemoriesStore = new MemoriesStoreOptionResponse() + MemoryStore = new MemoryStoreOptionResponse() { - Types = Enum.GetNames(typeof(MemoriesStoreOptions.MemoriesStoreType)), - SelectedType = this._memoriesStoreOptions.Type.ToString() + Types = Enum.GetNames(typeof(MemoryStoreOptions.MemoryStoreType)), + SelectedType = this._memoryStoreOptions.Type.ToString() } } ); diff --git a/webapi/Extensions/SemanticKernelExtensions.cs b/webapi/Extensions/SemanticKernelExtensions.cs index 82fce1721..034968784 100644 --- a/webapi/Extensions/SemanticKernelExtensions.cs +++ b/webapi/Extensions/SemanticKernelExtensions.cs @@ -22,7 +22,7 @@ using Microsoft.SemanticKernel.Orchestration; using Microsoft.SemanticKernel.Skills.Core; using Microsoft.SemanticKernel.TemplateEngine; -using static CopilotChat.WebApi.Options.MemoriesStoreOptions; +using static CopilotChat.WebApi.Options.MemoryStoreOptions; namespace CopilotChat.WebApi.Extensions; @@ -155,18 +155,18 @@ private static Task RegisterSkillsAsync(IServiceProvider sp, IKernel kernel) /// private static void AddSemanticTextMemory(this IServiceCollection services) { - MemoriesStoreOptions config = services.BuildServiceProvider().GetRequiredService>().Value; + MemoryStoreOptions config = services.BuildServiceProvider().GetRequiredService>().Value; switch (config.Type) { - case MemoriesStoreType.Volatile: + case MemoryStoreType.Volatile: services.AddSingleton(); break; - case MemoriesStoreType.Qdrant: + case MemoryStoreType.Qdrant: if (config.Qdrant == null) { - throw new InvalidOperationException("MemoriesStore type is Qdrant and Qdrant configuration is null."); + throw new InvalidOperationException("MemoryStore type is Qdrant and Qdrant configuration is null."); } services.AddSingleton(sp => @@ -189,10 +189,10 @@ private static void AddSemanticTextMemory(this IServiceCollection services) }); break; - case MemoriesStoreType.AzureCognitiveSearch: + case MemoryStoreType.AzureCognitiveSearch: if (config.AzureCognitiveSearch == null) { - throw new InvalidOperationException("MemoriesStore type is AzureCognitiveSearch and AzureCognitiveSearch configuration is null."); + throw new InvalidOperationException("MemoryStore type is AzureCognitiveSearch and AzureCognitiveSearch configuration is null."); } services.AddSingleton(sp => @@ -201,10 +201,10 @@ private static void AddSemanticTextMemory(this IServiceCollection services) }); break; - case MemoriesStoreOptions.MemoriesStoreType.Chroma: + case MemoryStoreOptions.MemoryStoreType.Chroma: if (config.Chroma == null) { - throw new InvalidOperationException("MemoriesStore type is Chroma and Chroma configuration is null."); + throw new InvalidOperationException("MemoryStore type is Chroma and Chroma configuration is null."); } services.AddSingleton(sp => @@ -222,7 +222,7 @@ private static void AddSemanticTextMemory(this IServiceCollection services) break; default: - throw new InvalidOperationException($"Invalid 'MemoriesStore' type '{config.Type}'."); + throw new InvalidOperationException($"Invalid 'MemoryStore' type '{config.Type}'."); } services.AddScoped(sp => new SemanticTextMemory( diff --git a/webapi/Extensions/ServiceExtensions.cs b/webapi/Extensions/ServiceExtensions.cs index 3e0f3fa09..e3a2fd966 100644 --- a/webapi/Extensions/ServiceExtensions.cs +++ b/webapi/Extensions/ServiceExtensions.cs @@ -53,8 +53,8 @@ public static IServiceCollection AddOptions(this IServiceCollection services, Co .PostConfigure(TrimStringProperties); // Memory store configuration - services.AddOptions() - .Bind(configuration.GetSection(MemoriesStoreOptions.PropertyName)) + services.AddOptions() + .Bind(configuration.GetSection(MemoryStoreOptions.PropertyName)) .ValidateDataAnnotations() .ValidateOnStart() .PostConfigure(TrimStringProperties); diff --git a/webapi/Models/Response/ServiceOptionsResponse.cs b/webapi/Models/Response/ServiceOptionsResponse.cs index e6a124409..d5eda9cba 100644 --- a/webapi/Models/Response/ServiceOptionsResponse.cs +++ b/webapi/Models/Response/ServiceOptionsResponse.cs @@ -9,25 +9,25 @@ namespace CopilotChat.WebApi.Models.Response; public class ServiceOptionsResponse { /// - /// The memories store that is configured. + /// Configured memory store. /// - [JsonPropertyName("memoriesStore")] - public MemoriesStoreOptionResponse MemoriesStore { get; set; } = new MemoriesStoreOptionResponse(); + [JsonPropertyName("memoryStore")] + public MemoryStoreOptionResponse MemoryStore { get; set; } = new MemoryStoreOptionResponse(); } /// -/// Response to memoriesStoreType request. +/// Response to memoryStoreType request. /// -public class MemoriesStoreOptionResponse +public class MemoryStoreOptionResponse { /// - /// All the available memories store types. + /// All the available memory store types. /// [JsonPropertyName("types")] public IEnumerable Types { get; set; } = Enumerable.Empty(); /// - /// The selected memories store type. + /// The selected memory store type. /// [JsonPropertyName("selectedType")] public string SelectedType { get; set; } = string.Empty; diff --git a/webapi/Options/MemoriesStoreOptions.cs b/webapi/Options/MemoriesStoreOptions.cs deleted file mode 100644 index 7486b4cba..000000000 --- a/webapi/Options/MemoriesStoreOptions.cs +++ /dev/null @@ -1,60 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -namespace CopilotChat.WebApi.Options; - -/// -/// Configuration settings for the memories store. -/// -public class MemoriesStoreOptions -{ - public const string PropertyName = "MemoriesStore"; - - /// - /// The type of memories store to use. - /// - public enum MemoriesStoreType - { - /// - /// Non-persistent memories store. - /// - Volatile, - - /// - /// Qdrant based persistent memories store. - /// - Qdrant, - - /// - /// Azure Cognitive Search persistent memories store. - /// - AzureCognitiveSearch, - - /// - /// Chroma DB persistent memories store. - /// - Chroma - } - - /// - /// Gets or sets the type of memories store to use. - /// - public MemoriesStoreType Type { get; set; } = MemoriesStoreType.Volatile; - - /// - /// Gets or sets the configuration for the Qdrant memories store. - /// - [RequiredOnPropertyValue(nameof(Type), MemoriesStoreType.Qdrant)] - public QdrantOptions? Qdrant { get; set; } - - /// - /// Gets or sets the configuration for the Chroma memories store. - /// - [RequiredOnPropertyValue(nameof(Type), MemoriesStoreType.Chroma)] - public VectorMemoryWebOptions? Chroma { get; set; } - - /// - /// Gets or sets the configuration for the Azure Cognitive Search memories store. - /// - [RequiredOnPropertyValue(nameof(Type), MemoriesStoreType.AzureCognitiveSearch)] - public AzureCognitiveSearchOptions? AzureCognitiveSearch { get; set; } -} diff --git a/webapi/Options/MemoryStoreOptions.cs b/webapi/Options/MemoryStoreOptions.cs new file mode 100644 index 000000000..a55da230a --- /dev/null +++ b/webapi/Options/MemoryStoreOptions.cs @@ -0,0 +1,60 @@ +// Copyright (c) Microsoft. All rights reserved. + +namespace CopilotChat.WebApi.Options; + +/// +/// Configuration settings for the memory store. +/// +public class MemoryStoreOptions +{ + public const string PropertyName = "MemoryStore"; + + /// + /// The type of memory store to use. + /// + public enum MemoryStoreType + { + /// + /// Non-persistent memory store. + /// + Volatile, + + /// + /// Qdrant based persistent memory store. + /// + Qdrant, + + /// + /// Azure Cognitive Search persistent memory store. + /// + AzureCognitiveSearch, + + /// + /// Chroma DB persistent memory store. + /// + Chroma + } + + /// + /// Gets or sets the type of memory store to use. + /// + public MemoryStoreType Type { get; set; } = MemoryStoreType.Volatile; + + /// + /// Gets or sets the configuration for the Qdrant memory store. + /// + [RequiredOnPropertyValue(nameof(Type), MemoryStoreType.Qdrant)] + public QdrantOptions? Qdrant { get; set; } + + /// + /// Gets or sets the configuration for the Chroma memory store. + /// + [RequiredOnPropertyValue(nameof(Type), MemoryStoreType.Chroma)] + public VectorMemoryWebOptions? Chroma { get; set; } + + /// + /// Gets or sets the configuration for the Azure Cognitive Search memory store. + /// + [RequiredOnPropertyValue(nameof(Type), MemoryStoreType.AzureCognitiveSearch)] + public AzureCognitiveSearchOptions? AzureCognitiveSearch { get; set; } +} diff --git a/webapi/Skills/ChatSkills/CopilotChatPlanner.cs b/webapi/Skills/ChatSkills/CopilotChatPlanner.cs index 9f4202b99..4841e0d55 100644 --- a/webapi/Skills/ChatSkills/CopilotChatPlanner.cs +++ b/webapi/Skills/ChatSkills/CopilotChatPlanner.cs @@ -136,7 +136,7 @@ public async Task RunStepwisePlannerAsync(string goal, SKContext cont } catch (Exception e) { - context.Log.LogError(e, "Error running stepwise planner"); + context.Logger.LogError(e, "Error running stepwise planner"); throw; } } diff --git a/webapi/appsettings.json b/webapi/appsettings.json index f7d7790f4..c4b65326d 100644 --- a/webapi/appsettings.json +++ b/webapi/appsettings.json @@ -127,12 +127,12 @@ // Memory stores are used for storing new memories and retrieving semantically similar memories. // - Supported Types are "volatile", "qdrant", "azurecognitivesearch", or "chroma". // - When using Qdrant or Azure Cognitive Search, see ./README.md for deployment instructions. - // - Set "MemoriesStore:AzureCognitiveSearch:Key" using dotnet's user secrets (see above) - // (i.e. dotnet user-secrets set "MemoriesStore:AzureCognitiveSearch:Key" "MY_AZCOGSRCH_KEY") - // - Set "MemoriesStore:Qdrant:Key" using dotnet's user secrets (see above) if you are using a Qdrant Cloud instance. - // (i.e. dotnet user-secrets set "MemoriesStore:Qdrant:Key" "MY_QDRANTCLOUD_KEY") + // - Set "MemoryStore:AzureCognitiveSearch:Key" using dotnet's user secrets (see above) + // (i.e. dotnet user-secrets set "MemoryStore:AzureCognitiveSearch:Key" "MY_AZCOGSRCH_KEY") + // - Set "MemoryStore:Qdrant:Key" using dotnet's user secrets (see above) if you are using a Qdrant Cloud instance. + // (i.e. dotnet user-secrets set "MemoryStore:Qdrant:Key" "MY_QDRANTCLOUD_KEY") // - "MemoriesStore": { + "MemoryStore": { "Type": "volatile", "Qdrant": { "Host": "http://localhost", diff --git a/webapp/src/components/chat/tabs/DocumentsTab.tsx b/webapp/src/components/chat/tabs/DocumentsTab.tsx index b4e89c315..fd0afefd3 100644 --- a/webapp/src/components/chat/tabs/DocumentsTab.tsx +++ b/webapp/src/components/chat/tabs/DocumentsTab.tsx @@ -181,14 +181,14 @@ export const DocumentsTab: React.FC = () => { {/* Hardcode vector database as we don't support switching vector store dynamically now. */}
- - {serviceOptions.memoriesStore.types.map((storeType) => { + + {serviceOptions.memoryStore.types.map((storeType) => { return ( ); })} diff --git a/webapp/src/libs/models/ServiceOptions.ts b/webapp/src/libs/models/ServiceOptions.ts index 79172ac5f..f275ba5fb 100644 --- a/webapp/src/libs/models/ServiceOptions.ts +++ b/webapp/src/libs/models/ServiceOptions.ts @@ -1,10 +1,10 @@ // Copyright (c) Microsoft. All rights reserved. -export interface MemoriesStore { +export interface MemoryStore { types: string[]; selectedType: string; } export interface ServiceOptions { - memoriesStore: MemoriesStore; + memoryStore: MemoryStore; } diff --git a/webapp/src/redux/features/app/AppState.ts b/webapp/src/redux/features/app/AppState.ts index 11cd7ecb0..1baa327ba 100644 --- a/webapp/src/redux/features/app/AppState.ts +++ b/webapp/src/redux/features/app/AppState.ts @@ -134,5 +134,5 @@ export const initialState: AppState = { tokenUsage: {}, features: Features, settings: Settings, - serviceOptions: { memoriesStore: { types: [], selectedType: '' } }, + serviceOptions: { memoryStore: { types: [], selectedType: '' } }, }; From 0031de6e29590c4a91814886a534f501b2c2c24e Mon Sep 17 00:00:00 2001 From: Mollie Munoz Date: Thu, 10 Aug 2023 09:37:26 -0700 Subject: [PATCH 4/5] Update Chat Copilot gif on primary Readme.md (#147) ### Motivation and Context A gif with better quality needed on primary Readme. ### Description Replaces current image with gif used on learn docs and semantic kernel primary Readme. ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [x] The PR follows the [Contribution Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone :smile: --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index bb02be1d1..5bff2ea9e 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ These quick-start instructions run the sample locally. To deploy the sample to A > **IMPORTANT:** Each chat interaction will call Azure OpenAI/OpenAI which will use tokens that you may be billed for. -![ChatCopilot](https://github.com/microsoft/chat-copilot/assets/64985898/4b5b4ddd-0ba5-4da1-9769-1bc4a74f1996) +![Chat Copilot answering a question](https://learn.microsoft.com/en-us/semantic-kernel/media/chat-copilot-in-action.gif) # Requirements From 7840959b639bc5bb8105a5cefbfac8ba76a1e15a Mon Sep 17 00:00:00 2001 From: Desmond Howard Date: Thu, 10 Aug 2023 17:07:18 -0400 Subject: [PATCH 5/5] Fix C# CodeQL alerts + add JS/TS to CodeQL (#139) ### Motivation and Context - changes the fix for the C# CodeQL alerts to be closer to the recommended solution since the code is still being flagged: https://github.com/microsoft/chat-copilot/security/code-scanning/7 my guess is that the alerts are still firing because the tool sees the input variable (`memoryName`) used on the same line as `LogWarning()` with no explicit call to `.Replace()`: image - updates the CodeQL config to also run on JS/TS files as well, which finds 2 alerts in the tests: https://github.com/dehoward/chat-copilot/security/code-scanning/6 ### Description ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [x] The PR follows the [Contribution Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone :smile: --- .github/workflows/codeql-analysis.yml | 61 +++++++++++----------- webapi/Controllers/ChatMemoryController.cs | 34 +++++------- webapp/tests/utils.ts | 4 +- 3 files changed, 45 insertions(+), 54 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index a360ada33..7ebf0c625 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -6,9 +6,9 @@ name: "CodeQL" on: push: - branches: [ "main", "experimental*", "feature*" ] + branches: ["main", "experimental*", "feature*"] schedule: - - cron: '17 11 * * 2' + - cron: "17 11 * * 2" jobs: analyze: @@ -22,45 +22,44 @@ jobs: strategy: fail-fast: false matrix: - language: [ 'csharp' ] + language: ["csharp", "javascript"] # CodeQL supports [ 'cpp', 'csharp', 'go', 'java', 'javascript', 'python', 'ruby' ] # Use only 'java' to analyze code written in Java, Kotlin or both # Use only 'javascript' to analyze code written in JavaScript, TypeScript or both # Learn more about CodeQL language support at https://aka.ms/codeql-docs/language-support steps: - - name: Checkout repository - uses: actions/checkout@v3 + - name: Checkout repository + uses: actions/checkout@v3 - # Initializes the CodeQL tools for scanning. - - name: Initialize CodeQL - uses: github/codeql-action/init@v2 - with: - languages: ${{ matrix.language }} - # If you wish to specify custom queries, you can do so here or in a config file. - # By default, queries listed here will override any specified in a config file. - # Prefix the list here with "+" to use these queries and those in the config file. + # Initializes the CodeQL tools for scanning. + - name: Initialize CodeQL + uses: github/codeql-action/init@v2 + with: + languages: ${{ matrix.language }} + # If you wish to specify custom queries, you can do so here or in a config file. + # By default, queries listed here will override any specified in a config file. + # Prefix the list here with "+" to use these queries and those in the config file. - # Details on CodeQL's query packs refer to : https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning#using-queries-in-ql-packs - # queries: security-extended,security-and-quality + # Details on CodeQL's query packs refer to : https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning#using-queries-in-ql-packs + # queries: security-extended,security-and-quality + # Autobuild attempts to build any compiled languages (C/C++, C#, Go, or Java). + # If this step fails, then you should remove it and run the build manually (see below) + - name: Autobuild + uses: github/codeql-action/autobuild@v2 - # Autobuild attempts to build any compiled languages (C/C++, C#, Go, or Java). - # If this step fails, then you should remove it and run the build manually (see below) - - name: Autobuild - uses: github/codeql-action/autobuild@v2 + # ℹ️ Command-line programs to run using the OS shell. + # 📚 See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsrun - # ℹ️ Command-line programs to run using the OS shell. - # 📚 See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsrun + # If the Autobuild fails above, remove it and uncomment the following three lines. + # modify them (or add more) to build your code if your project, please refer to the EXAMPLE below for guidance. - # If the Autobuild fails above, remove it and uncomment the following three lines. - # modify them (or add more) to build your code if your project, please refer to the EXAMPLE below for guidance. + # - run: | + # echo "Run, Build Application using script" + # ./location_of_script_within_repo/buildscript.sh - # - run: | - # echo "Run, Build Application using script" - # ./location_of_script_within_repo/buildscript.sh - - - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v2 - with: - category: "/language:${{matrix.language}}" + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v2 + with: + category: "/language:${{matrix.language}}" diff --git a/webapi/Controllers/ChatMemoryController.cs b/webapi/Controllers/ChatMemoryController.cs index 19337509a..2fba99335 100644 --- a/webapi/Controllers/ChatMemoryController.cs +++ b/webapi/Controllers/ChatMemoryController.cs @@ -60,18 +60,23 @@ public async Task GetSemanticMemoriesAsync( [FromRoute] string chatId, [FromRoute] string memoryName) { + // Sanitize the log input by removing new line characters. + // https://github.com/microsoft/chat-copilot/security/code-scanning/1 + var sanitizedChatId = chatId.Replace(Environment.NewLine, string.Empty, StringComparison.Ordinal); + var sanitizedMemoryName = memoryName.Replace(Environment.NewLine, string.Empty, StringComparison.Ordinal); + // Make sure the chat session exists. if (!await this._chatSessionRepository.TryFindByIdAsync(chatId, v => _ = v)) { - this._logger.LogWarning("Chat session: {0} does not exist.", this.SanitizeLogInput(chatId)); - return this.BadRequest($"Chat session: {chatId} does not exist."); + this._logger.LogWarning("Chat session: {0} does not exist.", sanitizedChatId); + return this.BadRequest($"Chat session: {sanitizedChatId} does not exist."); } // Make sure the memory name is valid. - if (!this.ValidateMemoryName(memoryName)) + if (!this.ValidateMemoryName(sanitizedMemoryName)) { - this._logger.LogWarning("Memory name: {0} is invalid.", this.SanitizeLogInput(memoryName)); - return this.BadRequest($"Memory name: {memoryName} is invalid."); + this._logger.LogWarning("Memory name: {0} is invalid.", sanitizedMemoryName); + return this.BadRequest($"Memory name: {sanitizedMemoryName} is invalid."); } // Gather the requested semantic memory. @@ -79,7 +84,7 @@ public async Task GetSemanticMemoriesAsync( // Will use a dummy query since we don't care about relevance. An empty string will cause exception. // minRelevanceScore is set to 0.0 to return all memories. List memories = new(); - string memoryCollectionName = SemanticChatMemoryExtractor.MemoryCollectionName(chatId, memoryName); + string memoryCollectionName = SemanticChatMemoryExtractor.MemoryCollectionName(sanitizedChatId, sanitizedMemoryName); try { var results = semanticTextMemory.SearchAsync( @@ -95,7 +100,8 @@ public async Task GetSemanticMemoriesAsync( catch (SKException connectorException) { // A store exception might be thrown if the collection does not exist, depending on the memory store connector. - this._logger.LogError(connectorException, "Cannot search collection {0}", this.SanitizeLogInput(memoryCollectionName)); + var sanitizedMemoryCollectionName = memoryCollectionName.Replace(Environment.NewLine, string.Empty, StringComparison.Ordinal); + this._logger.LogError(connectorException, "Cannot search collection {0}", sanitizedMemoryCollectionName); } return this.Ok(memories); @@ -113,19 +119,5 @@ private bool ValidateMemoryName(string memoryName) return this._promptOptions.MemoryMap.ContainsKey(memoryName); } - /// - /// Sanitizes the log input by removing new line characters. - /// This helps prevent log forgery attacks from malicious text. - /// - /// - /// https://github.com/microsoft/chat-copilot/security/code-scanning/1 - /// - /// The input to sanitize. - /// The sanitized input. - private string SanitizeLogInput(string input) - { - return input.Replace(Environment.NewLine, string.Empty, StringComparison.Ordinal); - } - # endregion } diff --git a/webapp/tests/utils.ts b/webapp/tests/utils.ts index 0e584e86e..19bcf928d 100644 --- a/webapp/tests/utils.ts +++ b/webapp/tests/utils.ts @@ -13,7 +13,7 @@ export async function loginHelper(page, useraccount, password) { // Expect the page to contain a "Login" button. await page.getByRole('button').click(); // Clicking the login button should redirect to the login page. - await expect(page).toHaveURL(new RegExp('^' + process.env.REACT_APP_AAD_AUTHORITY)); + await expect(page).toHaveURL(process.env.REACT_APP_AAD_AUTHORITY); // Login with the test user. await page.getByPlaceholder('Email, phone, or Skype').click(); await page.getByPlaceholder('Email, phone, or Skype').fill(useraccount as string); @@ -36,7 +36,7 @@ export async function loginHelperAnotherUser(page, useraccount, password) { // Expect the page to contain a "Login" button. await page.getByRole('button').click(); // Clicking the login button should redirect to the login page. - await expect(page).toHaveURL(new RegExp('^' + process.env.REACT_APP_AAD_AUTHORITY)); + await expect(page).toHaveURL(process.env.REACT_APP_AAD_AUTHORITY); // Login with the another user account. await page.getByRole('button', { name: 'Use another account' }).click(); await page.getByPlaceholder('Email, phone, or Skype').click();