-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add ValidationOptions for lenient output schema validation #1260
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?
Add ValidationOptions for lenient output schema validation #1260
Conversation
26c6806
to
7722e38
Compare
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.
Hi @lorenss-m, thanks for sending this!
Seems reasonable to add this option given the spec only says clients "should" validate structured results against the output schema.
I'm curious tho, which examples of non-conforming outputs did you find / how bad were they?
cc/ @bhosmer-ant since authored initial support in #993
) | ||
else: | ||
try: | ||
validate(result.structuredContent, output_schema) |
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.
What error would None produce here, do we need the two branches?
src/mcp/client/session.py
Outdated
logger = logging.getLogger("client") | ||
|
||
|
||
class ValidationOptions(BaseModel): |
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.
Let's not introduce parameters that are BaseModel's please. The best experience is usually to have flat parameters.
82db20c
to
9a8592e
Compare
…boolean parameter
Added the validate_structured_outputs suggestion! |
We should make sure that this matches how other SDKs behave before we merge this. @modelcontextprotocol/sdk-maintainers . |
In the Go SDK, currently all schema validation is done on the server side. |
The C# SDK delegates the validation of the structured content to the MCP Host. This is mainly because there is no standard, built-in JSON Schema validator in .NET, so doing this in the client would require pulling in an external package, one of potentially several, which adds bulk to the SDK and usurps the MCP Host's choce of validation package. |
Anything else we should change here? Would love to get this rolled out, our library is forced to depend on our fork otherwise! |
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.
Looks like there isn't necessarily a single approach across SDKs. Go does server-side validation, C# delegates it to the host, with neither currently doing any validation within the client.
This PR allows optionally disabling client side validation specifically in Python, which thus seems like it would enable behaviors like in Go & C# at least (no client side validation).
Also matches the spec as pointed out by @ochafik above, so this seems OK to me to merge.
However, left some comments - especially the change to auth
seems unintentional.
def construct_redirect_uri(redirect_uri_base: str, **params: str | None) -> str: | ||
parsed_uri = urlparse(redirect_uri_base) | ||
query_params = [(k, v) for k, vs in parse_qs(parsed_uri.query) for v in vs] | ||
query_params = [(k, v) for k, vs in parse_qs(parsed_uri.query).items() for v in vs] |
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.
intentional? seems unrelated
f"Invalid structured content returned by tool {name}: {e}. Continuing without validation." | ||
) | ||
except SchemaError as e: | ||
# Schema errors are always raised - they indicate a problem with the schema itself |
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.
nit: remove redundant comment
if output_schema is not None: | ||
if result.structuredContent is None: | ||
raise RuntimeError(f"Tool {name} has an output schema but did not return structured content") | ||
try: | ||
validate(result.structuredContent, output_schema) | ||
except ValidationError as e: | ||
raise RuntimeError(f"Invalid structured content returned by tool {name}: {e}") | ||
except SchemaError as e: | ||
raise RuntimeError(f"Invalid schema for tool {name}: {e}") | ||
if self._validate_structured_outputs: |
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.
nit: getting quite heavily nested here, could at least partially simplify with
if output_schema is None:
return
if result.structuredContent is None:
# raise / log depending
# handle the base case of try / except with raise / log depending
Hi @lorenss-m are you planning to come back to this one? |
Motivation and Context
Currently, MCP clients raise
RuntimeError
when tools don't return structured content matching theiroutputSchema
. This breaks integrations with servers that have schema/implementation mismatches.This PR adds optional lenient validation that converts these errors to warnings, allowing clients to continue operating with imperfect servers.
How Has This Been Tested?
tests/client/test_validation_options.py
Breaking Changes
None. Default behavior unchanged. Lenient validation is opt-in:
Types of changes
Checklist
Additional context
ValidationOptions
class following existing patterns (e.g.,TransportSecuritySettings
)ClientSession.call_tool()
to check options before raising validation errorsstructured_output_lowlevel.py
by creating proper package structure