Skip to content

fix: Refactor validation and exception handling for improved readability#583

Open
Kanchan-Microsoft wants to merge 7 commits intodevfrom
code-quality
Open

fix: Refactor validation and exception handling for improved readability#583
Kanchan-Microsoft wants to merge 7 commits intodevfrom
code-quality

Conversation

@Kanchan-Microsoft
Copy link
Contributor

@Kanchan-Microsoft Kanchan-Microsoft commented Mar 27, 2026

Purpose

This pull request includes a variety of small code quality improvements, bug fixes, and minor refactoring across both the backend and frontend. The main focus is on improving code readability, removing unused code, fixing minor logic issues, and standardizing API usage. No major features or architectural changes are introduced.

Key improvements and fixes include:

Backend improvements and bug fixes:

  • Standardized file path construction with Path.Combine for system prompt files in both ChatHost and KernelMemory to prevent potential path issues across platforms.
  • Improved logic for chat session ID assignment in makeNewSession to use the class member consistently, ensuring correct session tracking.
  • Simplified and clarified validation checks in Chat.cs by using if (!validator.Validate(request).IsValid) instead of comparing to false.
  • Improved exception handling in KernelMemory.cs by suppressing unused exception variables and disabling specific warnings, ensuring the process stays alive and logs as needed.
  • Fixed logic for filtering documents by date in DocumentRepository.cs, ensuring the filter is applied correctly when an end date is provided.

Frontend cleanup and refactoring:

  • Removed unused imports and code from multiple files (App.tsx, AppRoutes.tsx, AppContext.tsx, documentResults.ts, documentsService.ts), reducing bundle size and improving maintainability.
  • Commented out the unused formatKeywords function in documentsService.ts rather than deleting it, preserving it for potential future use.
  • Cleaned up and simplified state management and unused variables in the ChatRoom component, removing unnecessary state and logic for improved performance and readability.

These changes collectively improve code clarity, robustness, and maintainability across the project.

Does this introduce a breaking change?

  • Yes
  • No

Golden Path Validation

  • I have tested the primary workflows (the "golden path") to ensure they function correctly without errors.

Deployment Validation

  • I have validated the deployment process successfully and all services are running as expected with this change.

What to Check

Verify that the following are valid

  • ...

Other Information

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 27, 2026
View reviewed changes
App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
connection.Dispose();
}
using SqlCommand command = connection.CreateCommand();
command.CommandText = sql;

Check failure

Code scanning / CodeQL

SQL query built from user-controlled sources High

This query depends on
this ASP.NET Core routing endpoint.
Loading
.
This query depends on
this ASP.NET Core routing endpoint.
Loading
.

Copilot Autofix

AI about 6 hours ago

General approach: because SQL Server does not allow table or index names to be parameterized, the correct mitigation is to fully sanitize and validate any user-controlled value used to form identifiers before it is concatenated into SQL. This is done by enforcing a strict pattern (allowlist of characters) and length constraints, and rejecting anything that doesn’t match. Once such a sanitizer is applied, using the sanitized value in interpolated SQL is safe from injection.

Best concrete fix here: strengthen NormalizeIndexName in SqlServerMemory.cs so that it enforces a very restrictive pattern for normalized index names, e.g., only lowercase letters, digits, and underscores, and a reasonable maximum length. Then ensure the CreateIndexAsync method continues to call NormalizeIndexName before using index in the interpolated SQL. CodeQL will see that the tainted value passes through a sanitizer that enforces an allowlist, and the resulting SQL will no longer be considered vulnerable.

Specific changes:

  • In App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs, in the NormalizeIndexName method (lines 689–699), after computing index = s_replaceIndexNameCharsRegex.Replace(...), add a strong validation step that checks index against a strict regex like ^[a-z0-9_]+$ and also enforces a maximum length. If the index name is invalid, throw a ConfigurationException (or similar) rather than proceeding.
  • Optionally (but safely) add a using System.Text.RegularExpressions; only if it is not already present; in this file it is already imported, so no change is needed.
  • No changes are required to the CreateIndexAsync method or the rest of the pipeline; they will transparently benefit from the strengthened NormalizeIndexName.

This maintains existing behavior (dynamic per-index tables) but ensures that any index coming from HTTP query parameters is converted into a safe, canonical identifier or rejected outright.

Suggested changeset 1
App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs b/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
--- a/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
+++ b/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
@@ -690,9 +690,22 @@
     {
         ArgumentNullExceptionEx.ThrowIfNullOrWhiteSpace(index, nameof(index), "The index name is empty");
 
+        // Normalize: trim, lowercase, and replace any invalid characters with the valid separator.
         index = s_replaceIndexNameCharsRegex.Replace(index.Trim().ToLowerInvariant(), ValidSeparator);
 
-        // Validate the normalized index name
+        // Enforce a strict allowlist for the final index name: only lowercase letters, digits and underscore,
+        // and a reasonable maximum length to avoid abuse.
+        // This ensures the name is safe to interpolate into SQL identifiers.
+        if (string.IsNullOrWhiteSpace(index)
+            || index.Length > 128
+            || !Regex.IsMatch(index, "^[a-z0-9_]+$"))
+        {
+            throw new ConfigurationException($"The index name '{index}' is invalid. " +
+                "Index names may contain only lowercase letters, digits and underscore, " +
+                "must not be empty, and must be at most 128 characters long.");
+        }
+
+        // Additional validation hook (e.g., for backward compatibility or extra rules).
         ValidateIndexName(index);
 
         return index;
EOF
@@ -690,9 +690,22 @@
{
ArgumentNullExceptionEx.ThrowIfNullOrWhiteSpace(index, nameof(index), "The index name is empty");

// Normalize: trim, lowercase, and replace any invalid characters with the valid separator.
index = s_replaceIndexNameCharsRegex.Replace(index.Trim().ToLowerInvariant(), ValidSeparator);

// Validate the normalized index name
// Enforce a strict allowlist for the final index name: only lowercase letters, digits and underscore,
// and a reasonable maximum length to avoid abuse.
// This ensures the name is safe to interpolate into SQL identifiers.
if (string.IsNullOrWhiteSpace(index)
|| index.Length > 128
|| !Regex.IsMatch(index, "^[a-z0-9_]+$"))
{
throw new ConfigurationException($"The index name '{index}' is invalid. " +
"Index names may contain only lowercase letters, digits and underscore, " +
"must not be empty, and must be at most 128 characters long.");
}

// Additional validation hook (e.g., for backward compatibility or extra rules).
ValidateIndexName(index);

return index;
Copilot is powered by AI and may make mistakes. Always verify output.
App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
command.Parameters.AddWithValue("@index", index);
command.Parameters.AddWithValue("@key", record.Id);
using SqlCommand command = connection.CreateCommand();
command.CommandText = sql;

Check failure

Code scanning / CodeQL

SQL query built from user-controlled sources High

This query depends on
this ASP.NET Core routing endpoint.
Loading
.
This query depends on
this ASP.NET Core routing endpoint.
Loading
.

Copilot Autofix

AI about 6 hours ago

General approach: Since table names and schemas cannot be bound as SqlParameters, the correct fix is to ensure the index portion used in dynamic identifiers is strictly validated/sanitized so that only a constrained set of safe characters is allowed, and dangerous characters (such as ], ;, quotes, whitespace, etc.) cannot appear. This way, even though the index value comes from an HTTP query, it cannot affect SQL syntax beyond selecting from a known pattern of tables.

Best concrete fix here: Strengthen NormalizeIndexName in SqlServerMemory.cs so it enforces a clear, strict pattern on index names (for example, lowercase letters, digits, and underscores only, with a reasonable length limit). We can do this using the existing s_replaceIndexNameCharsRegex along with an explicit post-validation regex check, throwing an exception if the normalized index name is invalid. This makes it clear to both humans and static analyzers that index cannot contain SQL metacharacters by the time it is interpolated into table names.

The minimal change within the provided snippets is to update the NormalizeIndexName method in SqlServerMemory.cs to:

  • Keep trimming and lowercasing.
  • Use a regex to enforce that the resulting index matches a safe pattern like ^[a-z0-9_]+$.
  • Optionally enforce a maximum length (e.g., 128 or 64 characters) to match SQL identifier limits.
  • Throw a descriptive exception if validation fails.

No changes are needed to the call site of NormalizeIndexName or to the SQL construction itself, and no new libraries are needed beyond System.Text.RegularExpressions, which is already imported.

Concretely:

  • In App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs, replace the body of NormalizeIndexName so that it:
    • Throws if index is null/whitespace (as it does now).
    • Applies s_replaceIndexNameCharsRegex and lowercases.
    • Uses an explicit regex (e.g., ^[a-z0-9_]+$) to validate the resulting string; if it fails, throws.
    • Optionally enforces a length limit consistent with SQL Server identifiers (e.g., 128 characters).

This ensures that by the time index is used in $"{this._config.EmbeddingsTableName}_{index}", it is no longer tainted with unsafe characters, addressing the SQL injection concern.

Suggested changeset 1
App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs b/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
--- a/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
+++ b/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
@@ -690,9 +690,24 @@
     {
         ArgumentNullExceptionEx.ThrowIfNullOrWhiteSpace(index, nameof(index), "The index name is empty");
 
+        // Normalize: trim, lowercase, and replace invalid characters with a safe separator.
         index = s_replaceIndexNameCharsRegex.Replace(index.Trim().ToLowerInvariant(), ValidSeparator);
 
-        // Validate the normalized index name
+        // Enforce a strict pattern for index names: only lowercase letters, digits, and underscores.
+        // This ensures the value is always safe to embed into SQL identifiers.
+        const int MaxIndexLength = 128; // SQL Server identifier limit is 128 characters.
+        if (index.Length == 0 || index.Length > MaxIndexLength)
+        {
+            throw new KernelMemoryException($"The normalized index name is invalid or too long (max {MaxIndexLength} characters).");
+        }
+
+        // Allow only characters that are safe in SQL identifiers.
+        if (!Regex.IsMatch(index, "^[a-z0-9_]+$"))
+        {
+            throw new KernelMemoryException("The index name contains invalid characters.");
+        }
+
+        // Validate the normalized index name with any additional internal checks.
         ValidateIndexName(index);
 
         return index;
EOF
@@ -690,9 +690,24 @@
{
ArgumentNullExceptionEx.ThrowIfNullOrWhiteSpace(index, nameof(index), "The index name is empty");

// Normalize: trim, lowercase, and replace invalid characters with a safe separator.
index = s_replaceIndexNameCharsRegex.Replace(index.Trim().ToLowerInvariant(), ValidSeparator);

// Validate the normalized index name
// Enforce a strict pattern for index names: only lowercase letters, digits, and underscores.
// This ensures the value is always safe to embed into SQL identifiers.
const int MaxIndexLength = 128; // SQL Server identifier limit is 128 characters.
if (index.Length == 0 || index.Length > MaxIndexLength)
{
throw new KernelMemoryException($"The normalized index name is invalid or too long (max {MaxIndexLength} characters).");
}

// Allow only characters that are safe in SQL identifiers.
if (!Regex.IsMatch(index, "^[a-z0-9_]+$"))
{
throw new KernelMemoryException("The index name contains invalid characters.");
}

// Validate the normalized index name with any additional internal checks.
ValidateIndexName(index);

return index;
Copilot is powered by AI and may make mistakes. Always verify output.
App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
connection.Dispose();
}
using SqlCommand command = connection.CreateCommand();
command.CommandText = sql;

Check failure

Code scanning / CodeQL

SQL query built from user-controlled sources High

This query depends on
this ASP.NET Core routing endpoint.
Loading
.
This query depends on
this ASP.NET Core routing endpoint.
Loading
.

Copilot Autofix

AI about 6 hours ago

Copilot could not generate an autofix suggestion

Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.

App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
Comment on lines +250 to +264
command.CommandText = $@"
WITH [filters] AS
(
SELECT
cast([filters].[key] AS NVARCHAR(256)) COLLATE SQL_Latin1_General_CP1_CI_AS AS [name],
cast([filters].[value] AS NVARCHAR(256)) COLLATE SQL_Latin1_General_CP1_CI_AS AS [value]
FROM openjson(@filters) [filters]
)
SELECT TOP (@limit)
{queryColumns}
FROM
{this.GetFullTableName(this._config.MemoryTableName)}
SELECT TOP (@limit)
{queryColumns}
FROM
{this.GetFullTableName(this._config.MemoryTableName)}
WHERE 1=1
AND {this.GetFullTableName(this._config.MemoryTableName)}.[collection] = @index
{this.GenerateFilters(index, command.Parameters, filters)};";
AND {this.GetFullTableName(this._config.MemoryTableName)}.[collection] = @index
{this.GenerateFilters(index, command.Parameters, filters)};";

Check failure

Code scanning / CodeQL

SQL query built from user-controlled sources High

This query depends on
this ASP.NET Core routing endpoint.
Loading
.
This query depends on
this ASP.NET Core routing endpoint.
Loading
.
This query depends on
this ASP.NET Core routing endpoint.
Loading
.

Copilot Autofix

AI about 6 hours ago

General approach: Ensure user-controlled index does not influence SQL text (table names, schema names, etc.) in a way that cannot be parameterized. Values that belong in predicates are already passed via SqlParameters, which is good; the remaining issue is the dynamic tags table name built as $"{this._config.TagsTableName}_{index}". The safest approach is to avoid putting index into any identifier at all. Instead, use a tags table name derived solely from trusted configuration (_config.TagsTableName and _config.Schema), and store the index as a column value (which is already done in the main memory table). That way, index stays a parameter, not part of the SQL grammar.

Concrete best fix here:

  1. In SqlServerMemory.GenerateFilters, stop constructing the tags table name with _{index}. Replace:

    FROM {this.GetFullTableName($"{this._config.TagsTableName}_{index}")} AS [tags]

    with a call to GetFullTableName using just the configured tags table name, e.g.:

    FROM {this.GetFullTableName(this._config.TagsTableName)} AS [tags]

    This removes user-controlled input from the table name. The rest of the query, including index and filter values, is already handled via parameters, so no additional parameterization is required for this alert.

  2. Since GetFullTableName already calls ValidateTableName(tableName), and _config.TagsTableName comes from configuration (trusted), this change ensures all dynamic identifiers are configuration-only, not user input. The existing index normalization and ValidateIndexName remain useful but are no longer relied upon to protect table-name construction.

No new helper methods or imports are needed; we only edit the relevant SQL snippet inside GenerateFilters.

Suggested changeset 1
App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs b/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
--- a/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
+++ b/App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
@@ -641,7 +641,7 @@
                 filterBuilder.Append(CultureInfo.CurrentCulture, $@"EXISTS (
                          SELECT
 	                        1
-                        FROM {this.GetFullTableName($"{this._config.TagsTableName}_{index}")} AS [tags]
+                        FROM {this.GetFullTableName(this._config.TagsTableName)} AS [tags]
                         WHERE
 	                        [tags].[memory_id] = {this.GetFullTableName(this._config.MemoryTableName)}.[id]
                             AND [name] = @filter_{i}_{j}_name
EOF
@@ -641,7 +641,7 @@
filterBuilder.Append(CultureInfo.CurrentCulture, $@"EXISTS (
SELECT
1
FROM {this.GetFullTableName($"{this._config.TagsTableName}_{index}")} AS [tags]
FROM {this.GetFullTableName(this._config.TagsTableName)} AS [tags]
WHERE
[tags].[memory_id] = {this.GetFullTableName(this._config.MemoryTableName)}.[id]
AND [name] = @filter_{i}_{j}_name
Copilot is powered by AI and may make mistakes. Always verify output.
Copilot AI reviewed Mar 27, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR focuses on broad code-quality refactoring across backend (.NET / KernelMemory services) and frontend (React/TypeScript), aiming to improve readability, reduce unused code, and tighten some exception handling and filtering logic.

Changes:

  • Backend: refactors disposal/exception-handling patterns, improves path construction, and fixes/clarifies a date-filter application in DocumentRepository.
  • KernelMemory: various readability/perf cleanups (e.g., StringBuilder reuse, TryGetValue, using-statements).
  • Frontend/E2E: removes unused imports/variables and simplifies component state where setters were unused.

Reviewed changes

Copilot reviewed 59 out of 59 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
tests/e2e-test/tests/test_dkm_functional.py Removes unused Playwright import.
tests/e2e-test/tests/conftest.py Removes unused time import.
tests/e2e-test/pages/loginPage.py Calls BasePage constructor in page object.
tests/e2e-test/pages/dkmPage.py Calls BasePage constructor; removes dead commented code; narrows bare except.
App/kernel-memory/service/Service/ServiceConfiguration.cs Simplifies embedding-generation configuration validation logic.
App/kernel-memory/service/Service.AspNetCore/WebAPIEndpoints.cs Refines download endpoint exception handling with more specific catches.
App/kernel-memory/service/Core/Pipeline/Queue/DevTools/SimpleQueues.cs Narrows exception catches while polling filesystem queue.
App/kernel-memory/service/Core/MemoryStorage/DevTools/SimpleVectorDb.cs Uses TryGetValue for tag lookup in filter matching.
App/kernel-memory/service/Core/Handlers/SummarizationParallelHandler.cs Reuses StringBuilder across loop iterations.
App/kernel-memory/service/Core/Handlers/SummarizationHandler.cs Reuses StringBuilder across loop iterations.
App/kernel-memory/service/Core/Handlers/KeywordExtractingHandler.cs Removes unused variables; scopes extracted content variable.
App/kernel-memory/service/Core/FileSystem/DevTools/VolatileFileSystem.cs Minor file formatting/encoding change.
App/kernel-memory/service/Core/FileSystem/DevTools/DiskFileSystem.cs Ensures non-integer arithmetic for retry delay.
App/kernel-memory/service/Core/DataFormats/Office/MsWordDecoder.cs Uses using var to ensure document disposal.
App/kernel-memory/service/Abstractions/Pipeline/DataPipeline.cs Simplifies list copy; clarifies DocumentId validation rule for index deletion.
App/kernel-memory/service/Abstractions/Models/SearchResult.cs Removes redundant private setter from computed property.
App/kernel-memory/service/Abstractions/MemoryStorage/MemoryRecord.cs Removes redundant assignment during schema upgrade.
App/kernel-memory/service/Abstractions/Diagnostics/ArgumentOutOfRangeExceptionEx.cs Adjusts zero-check logic for float/double.
App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs Refactors connection/command disposal to using statements.
App/kernel-memory/extensions/Postgres/Postgres/PostgresMemory.cs Reuses StringBuilder; removes unused pattern variable.
App/kernel-memory/extensions/Postgres/Postgres/Internals/PostgresDbClient.cs Narrows exception handling and rethrows domain exceptions.
App/kernel-memory/extensions/Elasticsearch/Elasticsearch/Internals/ElasticsearchMemoryRecord.cs Removes unused local variable.
App/kernel-memory/extensions/Elasticsearch/Elasticsearch/ElasticsearchMemory.cs Drops unused response variables while still awaiting calls.
App/kernel-memory/extensions/AzureQueues/AzureQueuesPipeline.cs Ensures non-integer arithmetic for backoff delay.
App/kernel-memory/clients/dotnet/WebClient/MemoryWebClient.cs Simplifies parsing logic; refactors using scopes in multipart upload.
App/frontend-app/src/utils/httpClient/httpClient.ts Removes unused ApiError import.
App/frontend-app/src/utils/auth/roles.ts Removes unused isLoggedIn helper.
App/frontend-app/src/pages/home/home.tsx Removes unused imports/handlers/state setters; minor formatting cleanup.
App/frontend-app/src/components/uploadButton/uploadButton.tsx Removes unused icon imports.
App/frontend-app/src/components/sidecarCopilot/sidecar.tsx Removes unused state setters/options state.
App/frontend-app/src/components/searchResult/searchResultCard.tsx Removes unused imports/types.
App/frontend-app/src/components/searchResult/old.tsx Removes unused imports/vars; comments out unused download handler.
App/frontend-app/src/components/searchBox/searchBox.tsx Removes unused icons/components and dead handlers.
App/frontend-app/src/components/pagination/pagination.tsx Removes unused useState import.
App/frontend-app/src/components/layout/layout.tsx Removes unused Footer import.
App/frontend-app/src/components/headerBar/headerBar.tsx Removes unused admin/sign-in logic.
App/frontend-app/src/components/filter/filter.tsx Removes unused Button/t usage; keeps filter logic unchanged.
App/frontend-app/src/components/documentViewer/metadataTable.tsx Simplifies object check in metadata rendering.
App/frontend-app/src/components/documentViewer/imageCarousel.tsx Removes unused imports for navigation/buttons.
App/frontend-app/src/components/documentViewer/documentViewer.tsx Removes unused imports/state; keeps viewer structure.
App/frontend-app/src/components/datePicker/dateFilterDropdownMenu.tsx Removes unused CustomDatePicker import.
App/frontend-app/src/components/chat/optionsPanel.tsx Removes unused Label import.
App/frontend-app/src/components/chat/modelSwitch.tsx Removes unused code paths/state for a currently disabled UI.
App/frontend-app/src/components/chat/chatRoom.tsx Removes unused imports/state; comments out unused feedback handler.
App/frontend-app/src/components/chat/FeedbackForm.tsx Removes unused ChatMessage import.
App/frontend-app/src/api/documentsService.ts Removes unused type import; comments out unused helper function.
App/frontend-app/src/api/apiTypes/documentResults.ts Removes unused Facets import.
App/frontend-app/src/AppRoutes.tsx Removes unused React state/types.
App/frontend-app/src/AppContext.tsx Removes unused type import.
App/frontend-app/src/App.tsx Removes unused telemetry/app insights imports; simplifies provider usage.
App/backend-api/Microsoft.GS.DPS/Storage/Documents/DocumentRepository.cs Fixes/clarifies date filtering by correctly AND-ing into existing filter definition.
App/backend-api/Microsoft.GS.DPS/Storage/Components/BusinessTransactionRepository.cs Removes unused collection variable.
App/backend-api/Microsoft.GS.DPS/Storage/AISearch/TagUpdater.cs Simplifies raw response logging.
App/backend-api/Microsoft.GS.DPS/API/KernelMemory/KernelMemory.cs Uses platform-safe Path.Combine; suppresses unused exception variable.
App/backend-api/Microsoft.GS.DPS/API/ChatHost/ChatHost.cs Uses Path.Combine correctly; refactors session id assignment in makeNewSession.
App/backend-api/Microsoft.GS.DPS.Host/DependencyConfiguration/ServiceDependencies.cs Removes unused variable in Kernel registration.
App/backend-api/Microsoft.GS.DPS.Host/API/UserInterface/UserInterface.cs Uses TryGetValue for thumbnail lookup.
App/backend-api/Microsoft.GS.DPS.Host/API/KernelMemory/KernelMemory.cs Adds pragma for CA1031; removes unused assignment in status loop.
App/backend-api/Microsoft.GS.DPS.Host/API/ChatHost/Chat.cs Simplifies FluentValidation checks.
Comments suppressed due to low confidence (1)

App/backend-api/Microsoft.GS.DPS/API/ChatHost/ChatHost.cs:89

  • ChatHost is registered as a singleton, but makeNewSession mutates instance fields (this.sessionId, this.chatHistory, and this.chatSession is also set in Chat). This can cause cross-request/session state leakage and race conditions under concurrent requests. Prefer making ChatHost scoped/transient or refactoring to keep per-request/session state in locals (or keyed by sessionId) rather than on the singleton instance.
        private async Task<ChatSession> makeNewSession(string? chatSessionId)
        {
            if(string.IsNullOrEmpty(chatSessionId))
            {
                this.sessionId = Guid.NewGuid().ToString();
            }
            else
            {
                this.sessionId = chatSessionId;
            }

            //Create New Chat History
            this.chatHistory = new ChatHistory();
            //Add the system prompt to the chat history
            this.chatHistory.AddSystemMessage(ChatHost.s_systemPrompt);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

App/frontend-app/src/components/filter/filter.tsx Outdated
@@ -1,7 +1,7 @@
import React, { useEffect, useState, memo, useRef } from "react";
import { Checkbox } from "@fluentui/react-checkbox";
import { useTranslation } from "react-i18next";
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useTranslation is imported but no longer used in this component, which will trigger the repo’s @typescript-eslint/no-unused-vars warnings. Remove the unused import to keep lint clean.

Suggested change
import { useTranslation } from "react-i18next";

Copilot uses AI. Check for mistakes.
App/frontend-app/src/components/chat/modelSwitch.tsx
Comment on lines 1 to 10
import { useState } from "react";
import { Text, Tooltip } from "@fluentui/react-components";
import { useTranslation } from "react-i18next";

interface ModelSwitchProps {
onSwitchChange: (model: string) => void;
}

export function ModelSwitch({ onSwitchChange }: ModelSwitchProps) {
const { t } = useTranslation();
const GPT35 = "chat_35";
const GPT4 = "chat_4";
const GPT4O = "chat_4o";

Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useState, useTranslation, and the onSwitchChange prop are now unused, which will produce @typescript-eslint/no-unused-vars warnings. Remove the unused imports and consider prefixing the unused prop with _ (or remove it) until the UI is re-enabled.

Copilot uses AI. Check for mistakes.
App/frontend-app/src/components/documentViewer/documentViewer.tsx
Comment on lines 62 to 66
export function DocDialog(
{ metadata, isOpen, allChunkTexts, clearChatFlag, onClose, ...props }: DocDialogProps & Partial<TabListProps>
) {
const {t} = useTranslation();
const styles = useStyles();

Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useTranslation was removed from the component body, but the useTranslation import remains unused (will trigger @typescript-eslint/no-unused-vars warnings). Remove the unused import if translations aren’t needed here.

Copilot uses AI. Check for mistakes.
App/frontend-app/src/components/headerBar/headerBar.tsx
Comment on lines 58 to 61
const linkClasses = "cursor-pointer hover:no-underline hover:border-b-[3px] h-9 min-h-0 block text-white";
const linkCurrent = "pointer-events-none border-b-[3px]";
const isAuthenticated = accounts.length > 0;
const isAdmin = isPlatformAdmin(accounts);

Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After removing the admin/sign-in logic, isPlatformAdmin, Auth, and RedirectRequest imports appear unused in this file and will trigger @typescript-eslint/no-unused-vars warnings. Consider removing the unused imports (or reintroducing usage if still needed).

Copilot uses AI. Check for mistakes.
App/kernel-memory/service/Abstractions/Diagnostics/ArgumentOutOfRangeExceptionEx.cs
Comment on lines 196 to 201
public static void ThrowIfZero(double value, string paramName, string message)
{
if (value != 0) { return; }
if (Math.Abs(value) > 0) { return; }

throw new ArgumentOutOfRangeException(paramName, message);
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThrowIfZero(double) now uses Math.Abs(value) > 0 to detect non-zero. This will incorrectly throw for double.NaN (since Math.Abs(NaN) is NaN, and NaN > 0 is false). Use value != 0d or add an explicit NaN guard so NaN doesn't get treated as zero.

Copilot uses AI. Check for mistakes.
App/kernel-memory/service/Core/Pipeline/Queue/DevTools/SimpleQueues.cs
Comment on lines 201 to 237
#pragma warning disable CA1031 // need to log all errors
Task.Run(async () =>
{
await s_lock.WaitAsync().ConfigureAwait(false);
this._busy = true;
try
{
this._log.LogTrace("Populating queue {0}", this._queueName);
var messages = (await this._fileSystem.GetAllFileNamesAsync(this._queueName, "").ConfigureAwait(false)).ToList();
this._log.LogTrace("Queue {0}: {1} messages on disk", this._queueName, messages.Count);
foreach (var fileName in messages)
{
if (!fileName.EndsWith(FileExt, StringComparison.OrdinalIgnoreCase)) { continue; }

var messageId = fileName.Substring(0, fileName.Length - FileExt.Length);

// This check is not strictly required, only used to reduce logging statements
if (!this._messages.Contains(messageId))
{
this._log.LogTrace("Found message {0}", messageId);
this._messages.Add(messageId);
}
}
}
catch (DirectoryNotFoundException e)
{
this._log.LogError(e, "Directory missing, recreating");
await this._fileSystem.CreateVolumeAsync(this._queueName).ConfigureAwait(false);
}
catch (Exception e)
catch (IOException e)
{
this._log.LogError(e, "Unexpected error while polling the queue");
}
catch (UnauthorizedAccessException e)
{
this._log.LogError(e, "Unexpected error while polling the queue");
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#pragma warning disable CA1031 says the code needs to log all errors, but PopulateQueue no longer catches Exception—only a couple of specific exception types. Either add a final catch (Exception e) to log unexpected failures (important since this is inside Task.Run) or drop the pragma/comment and ensure exceptions are still observed/logged elsewhere.

Copilot uses AI. Check for mistakes.
App/backend-api/Microsoft.GS.DPS.Host/API/KernelMemory/KernelMemory.cs Outdated
Comment on lines 97 to 100
catch (Exception)
{
return Results.BadRequest(new DocumentDeletedResult() { IsDeleted = false });
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#pragma warning disable CA1031 is introduced here but never restored, so the warning stays disabled for the remainder of the file. Add a corresponding #pragma warning restore CA1031 after this catch block; also consider logging the caught exception so delete failures aren’t silent.

Suggested change
catch (Exception)
{
return Results.BadRequest(new DocumentDeletedResult() { IsDeleted = false });
}
catch (Exception ex)
{
app.Logger.LogError(ex, "An error occurred while deleting the document with ID {DocumentId}.", documentId);
return Results.BadRequest(new DocumentDeletedResult() { IsDeleted = false });
}
#pragma warning restore CA1031

Copilot uses AI. Check for mistakes.
App/backend-api/Microsoft.GS.DPS.Host/API/KernelMemory/KernelMemory.cs
Comment on lines 199 to 202
if (status.RemainingSteps.Count == 0)
{
completeFlag = true;
break;
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After removing completeFlag = true;, the completeFlag variable declared above is now unused. Consider removing the variable entirely (and any related logic) to avoid compiler warnings and keep the loop intent clear.

Copilot uses AI. Check for mistakes.
App/kernel-memory/service/Abstractions/Diagnostics/ArgumentOutOfRangeExceptionEx.cs
Comment on lines 145 to 149
public static void ThrowIfZero(float value, string paramName, string message)
{
if (value != 0) { return; }
if (Math.Abs(value) > 0) { return; }

throw new ArgumentOutOfRangeException(paramName, message);
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThrowIfZero(float) now uses Math.Abs(value) > 0 to detect non-zero. This will incorrectly throw for float.NaN because Math.Abs(NaN) is NaN and NaN > 0 is false. Consider reverting to value != 0f (which treats NaN as non-zero) or explicitly handling NaN before the zero check.

Copilot uses AI. Check for mistakes.
App/kernel-memory/extensions/SQLServer/SQLServer/SqlServerMemory.cs
Comment on lines +270 to +278
var dataReader = await command.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false);
while (await dataReader.ReadAsync(cancellationToken).ConfigureAwait(false))
{
command.Dispose();
connection.Dispose();
// Iterates over the entries and saves them in a list so that the connection can be closed before returning the results.
var entry = await this.ReadEntryAsync(dataReader, withEmbeddings, cancellationToken).ConfigureAwait(false);
list.Add(entry);
}

await dataReader.DisposeAsync().ConfigureAwait(false);
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: the dataReader in GetListAsync is disposed only at the end of the normal flow. Use await using (or try/finally) to ensure disposal on exceptions while reading/processing rows.

Copilot uses AI. Check for mistakes.
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 27, 2026
View reviewed changes
App/backend-api/Microsoft.GS.DPS.Host/API/KernelMemory/KernelMemory.cs
#pragma warning disable CA1031 // Must catch all to log and keep the process alive
catch (Exception ex)
{
app.Logger.LogError(ex, "An error occurred while deleting document {DocumentId}.", documentId);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.

Copilot Autofix

AI about 6 hours ago

In general: before logging user-provided data, normalize or sanitize it so that it cannot inject new log entries or otherwise confuse log consumers. For plain text logs, this typically means removing or replacing line breaks and other control characters from user input. For HTML logs, HTML-encode user input.

For this specific case, the best minimal fix is to sanitize documentId before including it in the log message. We will create a sanitized version of documentId that removes carriage returns and newlines (which are the primary vector for log forging in plain text logs) and then use that sanitized value in the call to LogError. This preserves the existing functionality (still logging the ID) while preventing an attacker from injecting additional log lines.

Concretely, in App/backend-api/Microsoft.GS.DPS.Host/API/KernelMemory/KernelMemory.cs, inside the catch (Exception ex) block of the MapDelete("/Documents/{documentId}", ...) handler, we will introduce a sanitizedDocumentId local variable that applies .Replace("\r", string.Empty).Replace("\n", string.Empty) to documentId, and then pass sanitizedDocumentId to LogError instead of documentId. No new imports are required.

Suggested changeset 1
App/backend-api/Microsoft.GS.DPS.Host/API/KernelMemory/KernelMemory.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/App/backend-api/Microsoft.GS.DPS.Host/API/KernelMemory/KernelMemory.cs b/App/backend-api/Microsoft.GS.DPS.Host/API/KernelMemory/KernelMemory.cs
--- a/App/backend-api/Microsoft.GS.DPS.Host/API/KernelMemory/KernelMemory.cs
+++ b/App/backend-api/Microsoft.GS.DPS.Host/API/KernelMemory/KernelMemory.cs
@@ -96,7 +96,11 @@
 #pragma warning disable CA1031 // Must catch all to log and keep the process alive
                 catch (Exception ex)
                 {
-                    app.Logger.LogError(ex, "An error occurred while deleting document {DocumentId}.", documentId);
+                    var sanitizedDocumentId = documentId?
+                        .Replace("\r", string.Empty)
+                        .Replace("\n", string.Empty);
+
+                    app.Logger.LogError(ex, "An error occurred while deleting document {DocumentId}.", sanitizedDocumentId);
                     return Results.BadRequest(new DocumentDeletedResult() { IsDeleted = false });
                 }
 #pragma warning restore CA1031
EOF
@@ -96,7 +96,11 @@
#pragma warning disable CA1031 // Must catch all to log and keep the process alive
catch (Exception ex)
{
app.Logger.LogError(ex, "An error occurred while deleting document {DocumentId}.", documentId);
var sanitizedDocumentId = documentId?
.Replace("\r", string.Empty)
.Replace("\n", string.Empty);

app.Logger.LogError(ex, "An error occurred while deleting document {DocumentId}.", sanitizedDocumentId);
return Results.BadRequest(new DocumentDeletedResult() { IsDeleted = false });
}
#pragma warning restore CA1031
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@Avijit-Microsoft Avijit-Microsoft Awaiting requested review from Avijit-Microsoft Avijit-Microsoft is a code owner

@Roopan-Microsoft Roopan-Microsoft Awaiting requested review from Roopan-Microsoft Roopan-Microsoft is a code owner

@Prajwal-Microsoft Prajwal-Microsoft Awaiting requested review from Prajwal-Microsoft Prajwal-Microsoft is a code owner

@Dongbumlee Dongbumlee Awaiting requested review from Dongbumlee Dongbumlee is a code owner

@Vinay-Microsoft Vinay-Microsoft Awaiting requested review from Vinay-Microsoft Vinay-Microsoft is a code owner

@aniaroramsft aniaroramsft Awaiting requested review from aniaroramsft aniaroramsft is a code owner

@toherman-msft toherman-msft Awaiting requested review from toherman-msft toherman-msft is a code owner

@nchandhi nchandhi Awaiting requested review from nchandhi nchandhi is a code owner

@dgp10801 dgp10801 Awaiting requested review from dgp10801 dgp10801 is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Kanchan-Microsoft