Skip to content

Conversation

@gopitk
Copy link
Contributor

@gopitk gopitk commented Dec 11, 2025

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

  • [X ] 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

  • [ x] I have read the MCP Documentation
  • [x ] My code follows the repository's style guidelines
  • [x ] New and existing tests pass locally
  • [ x] I have added appropriate error handling
  • I have added or updated documentation as needed

Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this! LGTM.

result = self.fn_metadata.convert_result(result)

return result
except UrlElicitationRequiredError:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created an issue to potentially find a better way to raise this error, but don't want to block on this: #1788

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @felixweinberger . I considered raising it as McpError (with the specific url elicitation error code info) but thought that could break existing clients.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@felixweinberger felixweinberger merged commit 8ac0cab into modelcontextprotocol:main Dec 15, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants