Fix for Url Elicitation issue 1768 - #1780
Conversation
Sorry, something went wrong.
felixweinberger
left a comment
There was a problem hiding this comment.
Thanks for submitting this! LGTM.
Sorry, something went wrong.
| result = self.fn_metadata.convert_result(result) | ||
|
|
||
| return result | ||
| except UrlElicitationRequiredError: |
There was a problem hiding this comment.
Feels slightly awkward to have this feature specific exception at such a generic level - but can't think of a better way to achieve the intended design of 1036 right now (and also matches what TS SDK does).
Sorry, something went wrong.
There was a problem hiding this comment.
Created an issue to potentially find a better way to raise this error, but don't want to block on this: #1788
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks @felixweinberger . I considered raising it as McpError (with the specific url elicitation error code info) but thought that could break existing clients.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah that wouldn't be the right approach - I was thinking more something along the lines of having a propagate_error: bool on McpError for example and then have some gating inside that on whether to raise or pass, but that's definitely overengineering for one exception case imo.
Sorry, something went wrong.
Fix for #1768
Motivation and Context
Change is needed because UrlElicitationRequiredError was not being propagated as an exception
How Has This Been Tested?
Tested with the examples/snippets indicated in the original issue. Also added a unit test for this case.
Breaking Changes
N/A
Types of changes
Checklist