Skip to content

feat!: #697 added tool input arguments validation#873

Open
ashakirin wants to merge 8 commits intomodelcontextprotocol:mainfrom
ashakirin:feature/697-tools-input-validation
Open

feat!: #697 added tool input arguments validation#873
ashakirin wants to merge 8 commits intomodelcontextprotocol:mainfrom
ashakirin:feature/697-tools-input-validation

Conversation

@ashakirin
Copy link
Copy Markdown
Contributor

Added tool input arguments validation causes Tool Execution Error accordingly SEP-1303.
It is breaking change, because tool input validation is activated by default

@Kehrlann Kehrlann self-assigned this Mar 26, 2026
@Kehrlann Kehrlann added area/server P1 Significant bug affecting many users, highly requested feature v2 labels Mar 26, 2026
Copy link
Copy Markdown
Contributor

@Kehrlann Kehrlann left a comment

Choose a reason for hiding this comment

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

Now that I'm revisiting this, I'm wondering whether we really want this system property to toggle this on or off.

We're going to ship this in 2.0, let it be a breaking change - after all, servers MUST validate tool inputs.

(And we should probably revisit tool name validation too, then)

@Kehrlann Kehrlann added waiting for user Waiting for user feedback or more details breaking-change labels Apr 1, 2026
@ashakirin
Copy link
Copy Markdown
Contributor Author

@Kehrlann regarding toggling:

  • I think toggling makes sense for tool names, because validation is specified as SHOULD/SHOULD NOT in the spec;
  • For server tool input schema validation, I'm not sure about toggling. The spec says MUST, so without validation the implementation would be non-spec-compliant. On the other hand, toggling could make SDK 2.0 adoption easier for some use cases. I agree to remove it.

@Kehrlann
Copy link
Copy Markdown
Contributor

Kehrlann commented Apr 3, 2026

For toggling, I'm in favor of leaving an option to turn it off. But let's get rid of the system property - we don't use that pattern anywhere else in the codebase.
If users don't want validation, they should indicate that in the client.

@ashakirin
Copy link
Copy Markdown
Contributor Author

@Kehrlann: Got it now — you meant we should reconsider using system properties specifically for tool name and input schema validation, not as a full toggle. That definitely makes sense for the input schema validation: I will remove the system property, but keep the toggle in the server builder. For tool names, server builder settings should be enough as well; exposing it as a system property is more the responsibility of a framework as the SDK. WDYT?

@Kehrlann
Copy link
Copy Markdown
Contributor

Kehrlann commented Apr 7, 2026

For tool names, server builder settings should be enough as well; exposing it as a system property is more the responsibility of a framework as the SDK

Exactly.

For the tool names, you can do it in a separate PR.

ashakirin and others added 6 commits April 10, 2026 14:38
… causes tool execution error. Breaking change, because validation is activated by default
…alidator.java

Co-authored-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com>
…tValidationIntegrationTests.java

Co-authored-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com>
@ashakirin ashakirin force-pushed the feature/697-tools-input-validation branch from 73daf65 to c094c06 Compare April 10, 2026 12:39
@ashakirin
Copy link
Copy Markdown
Contributor Author

@Kehrlann: I reworked PR with Map<String, Object> input schema

@Kehrlann Kehrlann removed the waiting for user Waiting for user feedback or more details label Apr 13, 2026
@Kehrlann
Copy link
Copy Markdown
Contributor

Thanks for your contribution. For info I've simplified the integration test a bit.

@Kehrlann Kehrlann self-requested a review April 13, 2026 11:28
Kehrlann
Kehrlann previously approved these changes Apr 13, 2026
Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
@Kehrlann Kehrlann force-pushed the feature/697-tools-input-validation branch from 255f080 to d7a6858 Compare April 13, 2026 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/server breaking-change P1 Significant bug affecting many users, highly requested feature v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants