Handle requests for capabilty, when capability not present by johnkors · Pull Request #290 · modelcontextprotocol/csharp-sdk

johnkors · 2025-04-11T21:14:51Z

Motivation and Context

Some clients ignore the capabilities, and sends <capability>/list requests regardless (ex: Claude Desktop).

ex: Claude Desktop polls for /resources/list even though resources is missing from the capabilities. The C# SDK throws exceptions and logs as ERROR statements. This PR changes this to just return the MethodNotFound error code over JSON-RPC, and log as a warning instead of error.

Relevant discussion: #74 (comment)

How Has This Been Tested?

Use Claude Desktop

Verify that my server does not log errors when connect to Claude.

Ran all tests

Breaking Changes

No

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally (I don't have full framework, who does??)
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

halter73 · 2025-04-21T17:58:26Z

ensures the server does not log this as an Error log statement

This part seems good. I agree that the server should not emit error logs just because a client is behaving improperly.

notifies the client about the bad request using a notification message, instead of returning an error messages

This seems problematic. Have you tried calling ListResourcesAsync from our client when connected to an MCP server without a list resources capability? I suspect it might hang now that the server no longer produces this JsonRpcError:

csharp-sdk/src/ModelContextProtocol/Shared/McpSession.cs

Lines 159 to 165 in 0c9e91f

    
           await SendMessageAsync(new JsonRpcError 
        
           { 
        
               Id = request.Id, 
        
               JsonRpc = "2.0", 
        
               Error = detail, 
        
               RelatedTransport = request.RelatedTransport, 
        
           }, cancellationToken).ConfigureAwait(false);

johnkors · 2025-04-23T08:57:00Z

I suspect it might hang now that the server no longer produces this JsonRpcError:

Ah, ok! Thanks. So it might be better to do respond with an JsonRpcError instead of sending the LoggingMessageNotification then?

I cannot throw, since that's tied together with the problematic error log level.

RelatedTransport seems new, I'll rebase and add that as well.

+++ await SendMessageAsync(new JsonRpcError 
+++ { 
+++     Id = request.Id, 
+++     JsonRpc = "2.0", 
+++     Error = detail, 
+++     RelatedTransport = request.RelatedTransport,  
+++ }, cancellationToken).ConfigureAwait(false); 
--- await _transport.SendMessageAsync(new JsonRpcNotification
---{
---    JsonRpc = "2.0",
---    Method = NotificationMethods.LoggingMessageNotification,
---    Params = new JsonObject
---    {
---        ["message"] = $"{EndpointName} lacks capability to do {request.Method}",
---        ["requestId"] = request.Id.ToString(),
---        ["type"] = nameof(BadRequest)
---    }
---}, cancellationToken).ConfigureAwait(false);
---return;

Have you tried calling ListResourcesAsync from our client when connected to an MCP server without a list resources capability? I suspect it might hang now that the server no longer produces this JsonRpcError

No, I did not do a manual regression test. All automated tests pass when I do dotnet test at least. One unrelated test fail if I run the test suite in Rider, I'll do a PR to make that pass if you want.

Will also lower log level from err to wrn Scenario: ex: Claude Desktop polls for `/resources/list` even though `resources` is missing from the `capabilities`

johnkors · 2025-04-23T10:50:27Z

Updated the code to respond with JsonRpcError instead.

halter73 · 2025-04-23T19:05:56Z

How does this surface now if the client calls something like SendRequestAsync or ListResourcesAsync and the capability/handler is missing on a server written with this SDK? Can you add a test verifying what happens makes sense?

johnkors · 2025-04-25T07:45:08Z

Not sure I can assert that the log level is reduced, but I have not changed any behaviour. Is there a missing test from before, I'll happily add it. Where do you want it?

Before the change:

throw ex
log Err
return JsonRpcError to client

After the change

don't throw,
log wrn
return JsonRpcError to client

stephentoub · 2025-04-28T13:56:19Z

+                    Code = (int) McpErrorCode.MethodNotFound,
+                },
+                RelatedTransport = request.RelatedTransport
+            }, cancellationToken).ConfigureAwait(false);


What does this achieve that's not already achieved when the caller a frame or two up catches the exception and similarly sends back a JsonRpcError?

csharp-sdk/src/ModelContextProtocol/Shared/McpSession.cs

Lines 135 to 166 in e190844

catch (Exception ex)

{

// Only send responses for request errors that aren't user-initiated cancellation.

bool isUserCancellation =

ex is OperationCanceledException &&

!cancellationToken.IsCancellationRequested &&

combinedCts?.IsCancellationRequested is true;

if (!isUserCancellation && message is JsonRpcRequest request)

{

LogRequestHandlerException(EndpointName, request.Method, ex);

JsonRpcErrorDetail detail = ex is McpException mcpe ?

new()

{

Code = (int)mcpe.ErrorCode,

Message = mcpe.Message,

} :

new()

{

Code = (int)McpErrorCode.InternalError,

Message = "An error occurred.",

};

await SendMessageAsync(new JsonRpcError

{

Id = request.Id,

JsonRpc = "2.0",

Error = detail,

RelatedTransport = request.RelatedTransport,

}, cancellationToken).ConfigureAwait(false);

}

Is the sole purpose here to avoid an exception try/catch, or are you trying to achieve something else?

I think part of the goal is to get rid of the LogRequestHandlerException warning log, but I agree that instead special-casing McpExceptions with McpErrorCode.MethodNotFound in the existing catch to emit a lower-severity log makes more sense.

Yeah, lower the level from err to wrn is/was the intent.

Another option is to pass something in the Exception or fine-grained exception handling, but not sure how much you like passing state and using Exceptions for control flow.

Happy to change it.

stephentoub · 2025-04-28T18:02:28Z

Before the change:
log Err

Which log is still at the error level?

johnkors · 2025-04-29T13:32:55Z

Which log is still at the error level?

An exception thrown by the handler function (a tool implementation throwing an exception for example).

stephentoub · 2025-04-29T13:50:24Z

Which log is still at the error level?

An exception thrown by the handler function (a tool implementation throwing an exception for example).

Can you show me which one? It shouldn't be at the error level anymore, eg

csharp-sdk/src/ModelContextProtocol/Shared/McpSession.cs

Lines 717 to 718 in e190844

    
           [LoggerMessage(Level = LogLevel.Warning, Message = "{EndpointName} method '{Method}' request handler failed.")] 
        
           private partial void LogRequestHandlerException(string endpointName, string method, Exception exception);

johnkors · 2025-04-29T14:36:50Z

Ah, ok - so it's a warn. Then I don't know what other scenarios is an Exception. Other exceptions wasn't really the focus for this PR. I only wanted to avoid the error logs when clients ask for capabilities that are not available. See OP.

Seems like you did some changes 2 weeks a go, while I open this PR 3 weeks ago.

Maybe it's no longer an issue. I'll update and check.

stephentoub · 2025-04-29T14:38:23Z

Seems like you did some changes 2 weeks ago

Yes, in response to your request that the logging level be lowered to warning from error.

johnkors · 2025-04-29T18:44:48Z

Okaydokay, did not know your PR was related. Looks better!

Claude traffic generate logs with WRN instead of ERR, where my server has no resources defined.

INF ModelContextProtocol.Server.McpServermethod 'initialize' request handler called.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'initialize' request handler completed.
WRN ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) received request for method 'resources/list', but not handler is available.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'tools/list' request handler called.
WRN ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) received request for method 'resources/list', but not handler is available.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'tools/list' request handler called.
WRN ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'resources/list' request handler failed.
WRN ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'resources/list' request handler failed.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'tools/list' request handler completed.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'tools/list' request handler completed.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'prompts/list' request handler called.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'prompts/list' request handler completed.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'prompts/list' request handler called.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) method 'prompts/list' request handler completed.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) message processing canceled.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) shutting down.
INF ModelContextProtocol.Server.McpServer Client (claude-ai 0.1.0) shut down.
WRN ModelContextProtocol.Server.McpServerreceived request for method 'resources/list', but not handler is available.
WRN ModelContextProtocol.Server.McpServermethod 'resources/list' request handler failed.
INF ModelContextProtocol.Server.McpServermethod 'prompts/list' request handler called.
INF ModelContextProtocol.Server.McpServermethod 'prompts/list' request handler completed.
INF ModelContextProtocol.Server.McpServermethod 'tools/call' request handler called.
INF ModelContextProtocol.Server.McpServermethod 'tools/call' request handler completed.

johnkors force-pushed the feat/handle-missing-capability-reqs branch from ba89760 to 92f7112 Compare April 23, 2025 08:53

johnkors force-pushed the feat/handle-missing-capability-reqs branch from 92f7112 to 21833b9 Compare April 23, 2025 09:02

Handle requests for capability, when capability not present

91959fa

Will also lower log level from err to wrn Scenario: ex: Claude Desktop polls for `/resources/list` even though `resources` is missing from the `capabilities`

johnkors force-pushed the feat/handle-missing-capability-reqs branch from 21833b9 to 91959fa Compare April 23, 2025 09:04

johnkors marked this pull request as ready for review April 23, 2025 09:09

stephentoub reviewed Apr 28, 2025

View reviewed changes

johnkors closed this Apr 29, 2025

johnkors deleted the feat/handle-missing-capability-reqs branch May 2, 2025 09:08

	catch (Exception ex)
	{
	// Only send responses for request errors that aren't user-initiated cancellation.
	bool isUserCancellation =
	ex is OperationCanceledException &&
	!cancellationToken.IsCancellationRequested &&
	combinedCts?.IsCancellationRequested is true;

	if (!isUserCancellation && message is JsonRpcRequest request)
	{
	LogRequestHandlerException(EndpointName, request.Method, ex);

	JsonRpcErrorDetail detail = ex is McpException mcpe ?
	new()
	{
	Code = (int)mcpe.ErrorCode,
	Message = mcpe.Message,
	} :
	new()
	{
	Code = (int)McpErrorCode.InternalError,
	Message = "An error occurred.",
	};

	await SendMessageAsync(new JsonRpcError
	{
	Id = request.Id,
	JsonRpc = "2.0",
	Error = detail,
	RelatedTransport = request.RelatedTransport,
	}, cancellationToken).ConfigureAwait(false);
	}

Uh oh!

Conversation

johnkors commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

Uh oh!

halter73 commented Apr 21, 2025

Uh oh!

johnkors commented Apr 23, 2025

Uh oh!

johnkors commented Apr 23, 2025

Uh oh!

halter73 commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnkors commented Apr 25, 2025

Uh oh!

stephentoub Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

halter73 Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

johnkors Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Apr 28, 2025

Uh oh!

johnkors commented Apr 29, 2025

Uh oh!

stephentoub commented Apr 29, 2025

Uh oh!

johnkors commented Apr 29, 2025

Uh oh!

stephentoub commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnkors commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

johnkors commented Apr 11, 2025 •

edited

Loading

halter73 commented Apr 23, 2025 •

edited

Loading

johnkors Apr 29, 2025 •

edited

Loading

stephentoub commented Apr 29, 2025 •

edited

Loading

johnkors commented Apr 29, 2025 •

edited

Loading