-
Notifications
You must be signed in to change notification settings - Fork 574
Add support for URL mode elicitation #1021
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
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 |
|---|---|---|
|
|
@@ -80,7 +80,7 @@ private void RegisterHandlers(McpClientOptions options, NotificationHandlers not | |
| cancellationToken), | ||
| McpJsonUtilities.JsonContext.Default.CreateMessageRequestParams, | ||
| McpJsonUtilities.JsonContext.Default.CreateMessageResult); | ||
|
|
||
| _options.Capabilities ??= new(); | ||
| _options.Capabilities.Sampling ??= new(); | ||
| } | ||
|
|
@@ -106,7 +106,19 @@ private void RegisterHandlers(McpClientOptions options, NotificationHandlers not | |
| McpJsonUtilities.JsonContext.Default.ElicitResult); | ||
|
|
||
| _options.Capabilities ??= new(); | ||
| _options.Capabilities.Elicitation ??= new(); | ||
| if (_options.Capabilities.Elicitation is null) | ||
| { | ||
| // Default to supporting only form mode if not explicitly configured | ||
| _options.Capabilities.Elicitation = new() | ||
| { | ||
| Form = new(), | ||
| }; | ||
halter73 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| else if (_options.Capabilities.Elicitation.Form is null && _options.Capabilities.Elicitation.Url is null) | ||
| { | ||
| // If ElicitationCapability is set but both modes are null, default to form mode for backward compatibility | ||
| _options.Capabilities.Elicitation.Form = new(); | ||
| } | ||
|
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. Seems like this could be simplified slightly to: _options.Capabilities.Elicitation ??= new();
if (_options.Capabilities.Elicitation.Form is null &&
_options.Capabilities.Elicitation.Url is null)
{
// If both modes are null, default to form mode for backward compatibility.
_options.Capabilities.Elicitation.Form = new();
} |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,23 +181,30 @@ ex is OperationCanceledException && | |
| { | ||
| LogRequestHandlerException(EndpointName, request.Method, ex); | ||
|
|
||
| JsonRpcErrorDetail detail = ex is McpProtocolException mcpProtocolException ? | ||
| new() | ||
| JsonRpcErrorDetail detail = ex switch | ||
| { | ||
| UrlElicitationRequiredException urlException => new() | ||
| { | ||
| Code = (int)urlException.ErrorCode, | ||
| Message = urlException.Message, | ||
| Data = urlException.CreateErrorDataNode(), | ||
| }, | ||
| McpProtocolException mcpProtocolException => new() | ||
| { | ||
| Code = (int)mcpProtocolException.ErrorCode, | ||
| Message = mcpProtocolException.Message, | ||
| } : ex is McpException mcpException ? | ||
| new() | ||
| }, | ||
| McpException mcpException => new() | ||
| { | ||
|
|
||
| Code = (int)McpErrorCode.InternalError, | ||
| Message = mcpException.Message, | ||
| } : | ||
| new() | ||
| }, | ||
| _ => new() | ||
| { | ||
| Code = (int)McpErrorCode.InternalError, | ||
| Message = "An error occurred.", | ||
| }; | ||
| }, | ||
| }; | ||
|
|
||
| var errorMessage = new JsonRpcError | ||
| { | ||
|
|
@@ -452,7 +459,7 @@ public async Task<JsonRpcResponse> SendRequestAsync(JsonRpcRequest request, Canc | |
| if (response is JsonRpcError error) | ||
| { | ||
| LogSendingRequestFailed(EndpointName, request.Method, error.Error.Message, error.Error.Code); | ||
| throw new McpProtocolException($"Request failed (remote): {error.Error.Message}", (McpErrorCode)error.Error.Code); | ||
| throw CreateRemoteProtocolException(error); | ||
| } | ||
|
|
||
| if (response is JsonRpcResponse success) | ||
|
|
@@ -769,6 +776,20 @@ private static TimeSpan GetElapsed(long startingTimestamp) => | |
| return null; | ||
| } | ||
|
|
||
| private static McpProtocolException CreateRemoteProtocolException(JsonRpcError error) | ||
| { | ||
| string formattedMessage = $"Request failed (remote): {error.Error.Message}"; | ||
| var errorCode = (McpErrorCode)error.Error.Code; | ||
|
|
||
| if (errorCode == McpErrorCode.UrlElicitationRequired && | ||
| UrlElicitationRequiredException.TryCreateFromError(formattedMessage, error.Error, out var urlException)) | ||
| { | ||
| return urlException; | ||
| } | ||
|
|
||
| return new McpProtocolException(formattedMessage, errorCode); | ||
|
Contributor
Author
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. Another thing we could do in addition to or instead of throwing a
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. That seems better / more general-purpose than a custom exception type.
Contributor
Author
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. Do you not like the ability to specifically catch a
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. Maybe I've misunderstood. Why does someone need to catch it? Are we using it for control flow?
Contributor
Author
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. Yes. This is tested by UrlElicitationRequired_Exception_Propagates_To_Client. I don't have a proper client sample, but if I did, it would look a lot like the TypeScript one in https://github.com/modelcontextprotocol/typescript-sdk/blob/e6c71bbab1dff7bf0c84eee96e74ef87f82a1dbe/src/examples/client/elicitationUrlExample.ts#L711-L719.
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. Does that mean the mcp spec dictates that url elicitations be modeled as json rpc errors?
Contributor
Author
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. It's either an error or a request. Section 4.2 and 4.3 of the new elicitation spec go over the two flows for URL elicitation.
https://modelcontextprotocol.io/specification/2025-11-25/client/elicitation#message-flow
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. Ok, in that case it makes sense to keep the exception. I think it's unfortunate that elicitation requests are being modeled as errors. |
||
| } | ||
|
|
||
| [LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} message processing canceled.")] | ||
| private partial void LogEndpointMessageProcessingCanceled(string endpointName); | ||
|
|
||
|
|
||
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.
Optional capability? It's a little redundant as almost every capability is optional. Maybe just "Clients declare their support.." ?
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.