-
Notifications
You must be signed in to change notification settings - Fork 299
[MCP] Centralize JSON‑RPC error handling, standardize MCP response envelope, and add default anonymous role for stdio mode. #3035
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
base: main
Are you sure you want to change the base?
Changes from all commits
b783c73
6979a8d
d634f8e
264af55
a2d3b68
f642960
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| namespace Azure.DataApiBuilder.Mcp.Core | ||
| { | ||
| /// <summary> | ||
| /// JSON-RPC 2.0 standard error codes used by the MCP stdio server. | ||
| /// These values come from the JSON-RPC 2.0 specification and are shared | ||
| /// so they are not hard-coded throughout the codebase. | ||
| /// </summary> | ||
| internal static class JsonRpcErrorCodes | ||
| { | ||
| /// <summary> | ||
| /// Invalid JSON was received by the server. | ||
| /// An error occurred on the server while parsing the JSON text. | ||
| /// </summary> | ||
| public const int PARSEERROR = -32700; | ||
|
|
||
| /// <summary> | ||
| /// The JSON sent is not a valid Request object. | ||
| /// </summary> | ||
| public const int INVALIDREQUEST = -32600; | ||
|
|
||
| /// <summary> | ||
| /// The method does not exist / is not available. | ||
| /// </summary> | ||
| public const int METHODNOTFOUND = -32601; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please consider using existing format "SCREAMING_SNAKE_CASE or UPPER_SNAKE_CASE" for all constant variables. refer- ../src/Config/HealthCheck/HealthCheckConstants.cs |
||
|
|
||
| /// <summary> | ||
| /// Invalid method parameter(s). | ||
| /// </summary> | ||
| public const int INVALIDPARAMS = -32602; | ||
|
|
||
| /// <summary> | ||
| /// Internal JSON-RPC error. | ||
| /// </summary> | ||
| public const int INTERNALERROR = -32603; | ||
anushakolan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,7 @@ public async Task RunAsync(CancellationToken cancellationToken) | |
|
|
||
| if (line.Length > MAX_LINE_LENGTH) | ||
| { | ||
| WriteError(id: null, code: -32600, message: "Request too large"); | ||
| WriteError(id: null, code: JsonRpcErrorCodes.INVALIDREQUEST, message: "Request too large"); | ||
| continue; | ||
| } | ||
|
|
||
|
|
@@ -77,13 +77,13 @@ public async Task RunAsync(CancellationToken cancellationToken) | |
| catch (JsonException jsonEx) | ||
| { | ||
| Console.Error.WriteLine($"[MCP DEBUG] JSON parse error: {jsonEx.Message}"); | ||
| WriteError(id: null, code: -32700, message: "Parse error"); | ||
| WriteError(id: null, code: JsonRpcErrorCodes.PARSEERROR, message: "Parse error"); | ||
| continue; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Console.Error.WriteLine($"[MCP DEBUG] Unexpected error parsing request: {ex.Message}"); | ||
| WriteError(id: null, code: -32603, message: "Internal error"); | ||
| WriteError(id: null, code: JsonRpcErrorCodes.INTERNALERROR, message: "Internal error"); | ||
| continue; | ||
| } | ||
|
|
||
|
|
@@ -99,7 +99,7 @@ public async Task RunAsync(CancellationToken cancellationToken) | |
|
|
||
| if (!root.TryGetProperty("method", out JsonElement methodEl)) | ||
| { | ||
| WriteError(id, -32600, "Invalid Request"); | ||
| WriteError(id, JsonRpcErrorCodes.INVALIDREQUEST, "Invalid Request"); | ||
| continue; | ||
| } | ||
|
|
||
|
|
@@ -133,13 +133,13 @@ public async Task RunAsync(CancellationToken cancellationToken) | |
| return; | ||
|
|
||
| default: | ||
| WriteError(id, -32601, $"Method not found: {method}"); | ||
| WriteError(id, JsonRpcErrorCodes.METHODNOTFOUND, $"Method not found: {method}"); | ||
| break; | ||
| } | ||
| } | ||
| catch (Exception) | ||
| { | ||
| WriteError(id, -32603, "Internal error"); | ||
| WriteError(id, JsonRpcErrorCodes.INTERNALERROR, "Internal error"); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -158,32 +158,22 @@ public async Task RunAsync(CancellationToken cancellationToken) | |
| /// </remarks> | ||
| private void HandleInitialize(JsonElement? id) | ||
| { | ||
| // Extract the actual id value from the request | ||
| object? requestId = id.HasValue ? GetIdValue(id.Value) : null; | ||
|
|
||
| // Create the initialize response | ||
| var response = new | ||
| var result = new | ||
| { | ||
| jsonrpc = "2.0", | ||
| id = requestId, | ||
| result = new | ||
| protocolVersion = _protocolVersion, | ||
| capabilities = new | ||
| { | ||
| protocolVersion = _protocolVersion, | ||
| capabilities = new | ||
| { | ||
| tools = new { listChanged = true }, | ||
| logging = new { } | ||
| }, | ||
| serverInfo = new | ||
| { | ||
| name = "Data API Builder", | ||
| version = "1.0.0" | ||
| } | ||
| tools = new { listChanged = true }, | ||
| logging = new { } | ||
| }, | ||
| serverInfo = new | ||
| { | ||
| name = "SQL MCP Server", | ||
anushakolan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| version = "1.0.0" | ||
|
Comment on lines
+171
to
+172
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you please also update the name and version in ../src/Azure.DataApiBuilder.Mcp/Core/McpServerConfiguration.cs? for consistency, try to refactor it and use the same name and version through a configurable value or use the DAB version as the version will keep changing.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its not a different product, we can use the already configurable DAB version, please use ProductInfo.GetVersion function to get this version. |
||
| } | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(response); | ||
| Console.Out.WriteLine(json); | ||
| WriteResult(id, result); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -225,7 +215,7 @@ private async Task HandleCallToolAsync(JsonElement? id, JsonElement root, Cancel | |
| { | ||
| if (!root.TryGetProperty("params", out JsonElement @params) || @params.ValueKind != JsonValueKind.Object) | ||
| { | ||
| WriteError(id, -32602, "Missing params"); | ||
| WriteError(id, JsonRpcErrorCodes.INVALIDPARAMS, "Missing params"); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -247,14 +237,14 @@ private async Task HandleCallToolAsync(JsonElement? id, JsonElement root, Cancel | |
| if (string.IsNullOrWhiteSpace(toolName)) | ||
| { | ||
| Console.Error.WriteLine("[MCP DEBUG] callTool → missing tool name."); | ||
| WriteError(id, -32602, "Missing tool name"); | ||
| WriteError(id, JsonRpcErrorCodes.INVALIDPARAMS, "Missing tool name"); | ||
| return; | ||
| } | ||
|
|
||
| if (!_toolRegistry.TryGetTool(toolName!, out IMcpTool? tool) || tool is null) | ||
| { | ||
| Console.Error.WriteLine($"[MCP DEBUG] callTool → tool not found: {toolName}"); | ||
| WriteError(id, -32602, $"Tool not found: {toolName}"); | ||
| WriteError(id, JsonRpcErrorCodes.INVALIDPARAMS, $"Tool not found: {toolName}"); | ||
| return; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,9 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System; | ||
anushakolan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| using System.Collections.Generic; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using Microsoft.Extensions.Configuration; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Hosting; | ||
|
|
@@ -15,9 +19,9 @@ internal static class McpStdioHelper | |
| /// Determines if MCP stdio mode should be run based on command line arguments. | ||
| /// </summary> | ||
| /// <param name="args"> The command line arguments.</param> | ||
| /// <param name="mcpRole"> The role for MCP stdio mode, if specified.</param> | ||
| /// <returns></returns> | ||
| public static bool ShouldRunMcpStdio(string[] args, out string? mcpRole) | ||
| /// <param name="mcpRole"> The role for MCP stdio mode. When this method returns true, the role is guaranteed to be non-null.</param> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: should mention mcpRole defaults to anonymous |
||
| /// <returns>True when MCP stdio mode should be enabled; otherwise false.</returns> | ||
| public static bool ShouldRunMcpStdio(string[] args, [NotNullWhen(true)] out string? mcpRole) | ||
| { | ||
| mcpRole = null; | ||
|
|
||
|
|
@@ -43,6 +47,11 @@ public static bool ShouldRunMcpStdio(string[] args, out string? mcpRole) | |
| } | ||
| } | ||
|
|
||
| // Ensure that when MCP stdio is enabled, mcpRole is always non-null. | ||
| // This matches the NotNullWhen(true) contract and avoids nullable warnings | ||
| // for callers while still allowing an implicit default when no role is provided. | ||
| mcpRole ??= "anonymous"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this necessary? can we instead just initialize as |
||
|
|
||
| return true; | ||
| } | ||
|
|
||
|
|
@@ -76,17 +85,13 @@ public static bool RunMcpStdioHost(IHost host) | |
|
|
||
| foreach (Mcp.Model.IMcpTool tool in tools) | ||
| { | ||
| _ = tool.GetToolMetadata(); | ||
| registry.RegisterTool(tool); | ||
| } | ||
|
|
||
| IServiceScopeFactory scopeFactory = | ||
| host.Services.GetRequiredService<IServiceScopeFactory>(); | ||
| using IServiceScope scope = scopeFactory.CreateScope(); | ||
| IHostApplicationLifetime lifetime = | ||
| scope.ServiceProvider.GetRequiredService<IHostApplicationLifetime>(); | ||
| host.Services.GetRequiredService<IHostApplicationLifetime>(); | ||
| Mcp.Core.IMcpStdioServer stdio = | ||
| scope.ServiceProvider.GetRequiredService<Mcp.Core.IMcpStdioServer>(); | ||
| host.Services.GetRequiredService<Mcp.Core.IMcpStdioServer>(); | ||
|
|
||
| stdio.RunAsync(lifetime.ApplicationStopping).GetAwaiter().GetResult(); | ||
| host.StopAsync().GetAwaiter().GetResult(); | ||
|
|
||
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.
please consider renaming this to something like McpStdioJsonRpcErrorCodes or something to reflect its related to only MCP. also, we should move this inside "../src/Azure.DataApiBuilder.Mcp/Model" where we already have other error codes and enums, specifically for MCP.