-
Notifications
You must be signed in to change notification settings - Fork 0
Dev #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* Remove Azure Table Storage service registration Removed Azure Table Storage registration from services. * Refactor TableStorageService constructor for config Refactored constructor to initialize TableServiceClient based on environment variables for better configuration handling. * Refactor namespace and add Azure.Identity using * Move KnowledgeGraphService to Services namespace * Move namespace for RelationService to Services * Add using directives for Services and Storage * Add Storage namespace to RelationService * Add using directive for Storage namespace * Refactor GraphFunctions to use constructor parameters * Refactor relation ID handling in RelationService * Refactor RelationService and add TableStorageService * Refactor RelationService to implement IRelationService
* Update server version to 0.5.1 in host.json * Delete CHANGELOG.md
--- updated-dependencies: - dependency-name: Azure.Identity dependency-version: 1.17.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…orker.Extensions.Mcp (MWG-Logan#43) Bumps Microsoft.Azure.Functions.Worker from 2.50.0 to 2.51.0 Bumps Microsoft.Azure.Functions.Worker.Extensions.Mcp from 1.0.0 to 1.1.0 --- updated-dependencies: - dependency-name: Microsoft.Azure.Functions.Worker dependency-version: 2.51.0 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: Microsoft.Azure.Functions.Worker.Extensions.Mcp dependency-version: 1.1.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
There was a problem hiding this 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 performs a refactoring focused on namespace reorganization, managed identity support, and dependency updates. The changes migrate from constructor injection to a self-configuring TableStorageService that supports both connection string and managed identity authentication. However, the PR contains a critical bug where the entire RelationService.cs file is duplicated.
Key Changes:
- Reorganized codebase into
CentralMemoryMcp.Functions.ServicesandCentralMemoryMcp.Functions.Storagenamespaces - Added managed identity authentication support via
DefaultAzureCredentialas a fallback to connection strings - Updated primary constructor syntax in GraphFunctions and removed explicit DI registration for TableServiceClient in Program.cs
- Updated NuGet packages to newer versions
- Changed dependabot configuration from npm to nuget ecosystem
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| CentralMemoryMcp.Functions/host.json | Version downgraded from 1.0.0 to 0.5.1 and whitespace formatting fix |
| CentralMemoryMcp.Functions/Storage/TableStorageService.cs | Moved to Storage namespace, added parameterless constructor with managed identity fallback, removed Azure using directive |
| CentralMemoryMcp.Functions/Services/RelationService.cs | CRITICAL BUG: Entire file duplicated starting at line 127; namespace updated to Services; minor code cleanups with string interpolation |
| CentralMemoryMcp.Functions/Services/KnowledgeGraphService.cs | Namespace updated to Services, Storage namespace import added |
| CentralMemoryMcp.Functions/Program.cs | Removed explicit TableServiceClient registration, updated namespace imports |
| CentralMemoryMcp.Functions/Functions/GraphFunctions.cs | Updated to primary constructor syntax, inconsistent field storage pattern (graph vs _relations), removed unused System comment |
| CentralMemoryMcp.Functions/CentralMemoryMcp.Functions.csproj | Updated package versions: Worker 2.50.0→2.51.0, MCP 1.0.0→1.1.0, Identity 1.17.0→1.17.1 |
| CHANGELOG.md | File deleted entirely |
| .github/dependabot.yml | Changed package ecosystem from "npm" to "nuget" |
Comments suppressed due to low confidence (1)
CentralMemoryMcp.Functions/Services/RelationService.cs:252
- The entire file content is duplicated starting from line 127. Lines 127-252 are an exact duplicate of lines 1-126. This will cause compilation errors as it creates duplicate class and interface definitions. The duplicate code should be removed.
using Azure;
using Azure.Data.Tables;
using CentralMemoryMcp.Functions.Models;
using CentralMemoryMcp.Functions.Storage;
namespace CentralMemoryMcp.Functions.Services;
public interface IRelationService
{
Task<RelationModel> UpsertRelationAsync(RelationModel model, CancellationToken ct = default);
Task<RelationModel?> GetRelationAsync(string workspaceName, Guid relationId, CancellationToken ct = default);
Task<List<RelationModel>> GetRelationsFromEntityAsync(string workspaceName, Guid fromEntityId, CancellationToken ct = default);
Task<List<RelationModel>> GetRelationsForWorkspaceAsync(string workspaceName, CancellationToken ct = default);
Task DeleteRelationAsync(string workspaceName, Guid relationId, CancellationToken ct = default);
}
public class RelationService(ITableStorageService storage) : IRelationService
{
public async Task<RelationModel> UpsertRelationAsync(RelationModel model, CancellationToken ct = default)
{
var table = await storage.GetRelationsTableAsync(ct);
// Check for existing relation (same workspace, from, to, type)
var filter = $"PartitionKey eq '{model.WorkspaceName}' and FromEntityId eq '{model.FromEntityId:N}' and ToEntityId eq '{model.ToEntityId:N}' and RelationType eq '{EscapeFilterValue(model.RelationType)}'";
await foreach(var e in table.QueryAsync<TableEntity>(filter: filter, maxPerPage:1, cancellationToken: ct))
{
// Reuse its Id
if (e.TryGetValue("Id", out var idObj) && idObj is string idStr && Guid.TryParse(idStr, out var existingId))
{
model.Id = existingId;
}
break;
}
var entity = new TableEntity(model.PartitionKey, model.RowKey)
{
{"Id", model.Id.ToString("N")},
{"WorkspaceName", model.WorkspaceName},
{"FromEntityId", model.FromEntityId.ToString("N")},
{"ToEntityId", model.ToEntityId.ToString("N")},
{"RelationType", model.RelationType},
{"Metadata", model.Metadata ?? string.Empty}
};
await table.UpsertEntityAsync(entity, TableUpdateMode.Replace, ct);
return model;
}
public async Task<RelationModel?> GetRelationAsync(string workspaceName, Guid relationId, CancellationToken ct = default)
{
var table = await storage.GetRelationsTableAsync(ct);
try
{
var response = await table.GetEntityAsync<TableEntity>(workspaceName, relationId.ToString("N"), cancellationToken: ct);
var model = new RelationModel(
response.Value.GetString("WorkspaceName")!,
Guid.Parse(response.Value.GetString("FromEntityId")!),
Guid.Parse(response.Value.GetString("ToEntityId")!),
response.Value.GetString("RelationType")!,
response.Value.GetString("Metadata"))
{
Id = relationId
};
return model;
}
catch (RequestFailedException ex) when (ex.Status == 404)
{
return null;
}
}
public async Task<List<RelationModel>> GetRelationsFromEntityAsync(string workspaceName, Guid fromEntityId, CancellationToken ct = default)
{
var table = await storage.GetRelationsTableAsync(ct);
var results = new List<RelationModel>();
var fromIdStr = fromEntityId.ToString("N");
await foreach (var e in table.QueryAsync<TableEntity>(
filter: $"PartitionKey eq '{workspaceName}' and FromEntityId eq '{fromIdStr}'",
cancellationToken: ct))
{
var relationId = Guid.TryParse(e.GetString("Id"), out var rid) ? rid : Guid.NewGuid();
var model = new RelationModel(
e.GetString("WorkspaceName")!,
Guid.Parse(e.GetString("FromEntityId")!),
Guid.Parse(e.GetString("ToEntityId")!),
e.GetString("RelationType")!,
e.GetString("Metadata"));
model.Id = relationId;
results.Add(model);
}
return results;
}
public async Task<List<RelationModel>> GetRelationsForWorkspaceAsync(string workspaceName, CancellationToken ct = default)
{
var table = await storage.GetRelationsTableAsync(ct);
var results = new List<RelationModel>();
await foreach (var e in table.QueryAsync<TableEntity>(
filter: $"PartitionKey eq '{workspaceName}'",
cancellationToken: ct))
{
var relationId = Guid.TryParse(e.GetString("Id"), out var rid) ? rid : Guid.NewGuid();
var model = new RelationModel(
e.GetString("WorkspaceName")!,
Guid.Parse(e.GetString("FromEntityId")!),
Guid.Parse(e.GetString("ToEntityId")!),
e.GetString("RelationType")!,
e.GetString("Metadata"));
model.Id = relationId;
results.Add(model);
}
return results;
}
public async Task DeleteRelationAsync(string workspaceName, Guid relationId, CancellationToken ct = default)
{
var table = await storage.GetRelationsTableAsync(ct);
try
{
await table.DeleteEntityAsync(workspaceName, relationId.ToString("N"), cancellationToken: ct);
}
catch (RequestFailedException ex) when (ex.Status == 404)
{
// not found; ignore
}
}
private static string EscapeFilterValue(string value) => value.Replace("'", "''");
}
| var table = await storage.GetRelationsTableAsync(ct); | ||
| // Check for existing relation (same workspace, from, to, type) | ||
| string filter = $"PartitionKey eq '{model.WorkspaceName}' and FromEntityId eq '{model.FromEntityId.ToString("N")}' and ToEntityId eq '{model.ToEntityId.ToString("N")}' and RelationType eq '{EscapeFilterValue(model.RelationType)}'"; | ||
| var filter = $"PartitionKey eq '{model.WorkspaceName}' and FromEntityId eq '{model.FromEntityId:N}' and ToEntityId eq '{model.ToEntityId:N}' and RelationType eq '{EscapeFilterValue(model.RelationType)}'"; |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter string uses model.WorkspaceName directly without escaping. Workspace names could contain single quotes that would break the OData filter syntax or potentially lead to filter injection. Consider using EscapeFilterValue() for all string values: var filter = $"PartitionKey eq '{EscapeFilterValue(model.WorkspaceName)}' and FromEntityId eq '{model.FromEntityId:N}' and ToEntityId eq '{model.ToEntityId:N}' and RelationType eq '{EscapeFilterValue(model.RelationType)}'";
| await foreach (var e in table.QueryAsync<TableEntity>( | ||
| filter: $"PartitionKey eq '{workspaceName}'", |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter string uses workspaceName directly without escaping. Workspace names could contain single quotes that would break the OData filter syntax or potentially lead to filter injection. Consider using EscapeFilterValue() for the workspace name: filter: $"PartitionKey eq '{EscapeFilterValue(workspaceName)}'"
| "instructions": "This server offers LLMs long-term memory storage capabilities about users, projects, and anything else needed. This should not entirely replace using public information tooling like Microsoft Docs or Context7.", | ||
| "serverName": "Central Memory MCP", | ||
| "serverVersion": "1.0.0", | ||
| "serverVersion": "0.5.1", |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The server version is being downgraded from "1.0.0" to "0.5.1", which seems unusual. Typically, versions move forward, not backward. This could cause confusion for API consumers and violate semantic versioning expectations. If this is intentional to reflect a pre-release state, please verify this is correct. Otherwise, consider versioning forward (e.g., "1.0.1" or "1.1.0").
| "serverVersion": "0.5.1", | |
| "serverVersion": "1.0.1", |
| public class GraphFunctions(IKnowledgeGraphService graph, IRelationService relations) | ||
| { | ||
| private readonly IKnowledgeGraphService _graph; | ||
| private readonly IRelationService _relations; | ||
|
|
||
| public GraphFunctions(IKnowledgeGraphService graph, IRelationService relations) | ||
| { | ||
| _graph = graph; | ||
| _relations = relations; | ||
| } | ||
| private readonly IRelationService _relations = relations; |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The graph parameter is used directly in method calls (lines 20, 51, 76, 81, 119) but is not stored in a field. However, a field _relations is created for the relations parameter at line 11. For consistency, either both should be stored as fields, or neither should be (using parameters directly throughout). The current approach is inconsistent.
| string? entityName) | ||
| { | ||
| Guid resolvedId = Guid.Empty; | ||
| Guid resolvedId; |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The variable resolvedId is declared without initialization. While it's assigned in all code paths before use, C# 14 flow analysis may flag this as potentially unassigned if the compiler cannot prove all paths assign it. Consider initializing to Guid.Empty for clarity: Guid resolvedId = Guid.Empty;
| Guid resolvedId; | |
| Guid resolvedId = Guid.Empty; |
| await foreach (var e in table.QueryAsync<TableEntity>( | ||
| filter: $"PartitionKey eq '{workspaceName}' and FromEntityId eq '{fromIdStr}'", |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter string uses model.WorkspaceName and fromIdStr directly without escaping. While GUIDs are safe, workspace names could contain single quotes that would break the OData filter syntax or potentially lead to filter injection. Consider using EscapeFilterValue() for model.WorkspaceName as well: filter: $"PartitionKey eq '{EscapeFilterValue(workspaceName)}' and FromEntityId eq '{fromIdStr}'"
No description provided.