feat: added tools name format validation accordingly #SEP-986

[ashakirin]

Added tools name format validation accordingly #SEP-986
Note: tool duplications are already checked (in McpAsyncServer.java and in McpStatelessAsyncServer.java)

[Kehrlann]

Thanks for your contribution @ashakirin ! A few comments.

Optional validation

The spec specifically says that tools SHOULD follow the naming conventions (Server > Tools > Tool names). They MAY not follow these, and so we should allow invalid names (although not by default).
We could consider a per-server validator, that you plug in the SyncSpecification and AsyncSpecification. Thoughts?

Client-side validation

I like the approach on the client side, validate but don't fail. The spec itself is super vague, "client can validate" which has no RFC meaning. Maybe we shouldn't even validate on the client side.

If we chose to validate, in the current implementation the log is very verbose if you're writing a client and the server (that you don't control) has an invalid tool name. Every time you make a "call tool request" you get a WARN-level log. I think that's too much, and the level is probably too high. In that case, keep the logs for the "list tool" request but not "call tool".

[ashakirin]

@Kehrlann: Thanks for detail review, Daniel!
My thoughts:

Optional validation

The spec specifically says that tools SHOULD follow the naming conventions (Server > Tools > Tool names). They MAY not follow these, and so we should allow invalid names (although not by default). We could consider a per-server validator, that you plug in the SyncSpecification and AsyncSpecification. Thoughts?

My initial interpretation of SHOULD was that the SDK may choose whether to support validation at all, not that validation of server-side tool names should be optional. That seems to be incorrect.

Proposal: using different validation options for different MCP servers registered in the same application is probably not a very typical use case. I would therefore propose a global setting to disable validation, with an option to override it per server in SyncSpecification, AsyncSpecification, StatelessAsyncSpecification, and StatelessSyncSpecification. WDYT?
This keeps the default behavior simple while still allowing flexibility when integrating with non-compliant servers. What do you think?

Client-side validation

I like the approach on the client side, validate but don't fail. The spec itself is super vague, "client can validate" which has no RFC meaning. Maybe we shouldn't even validate on the client side.

If we chose to validate, in the current implementation the log is very verbose if you're writing a client and the server (that you don't control) has an invalid tool name. Every time you make a "call tool request" you get a WARN-level log. I think that's too much, and the level is probably too high. In that case, keep the logs for the "list tool" request but not "call tool".

My idea was to validate tool names via deserialization on both the client and server sides: on the client during listTools, and on the server during tool/call.
I agree that the current implementation in compact constructors is too verbose, because:

• the client constructs the CallToolRequest itself
• the compact listTools constructor is invoked twice due calling of pagination / cursors constructor

Therefore I am going to move validation into McpAsyncClient.listToolsInternal().
Do you think it makes sense to validate tool names on the server side during tool/call? Basically a client can call server with non-existent and invalid tool name, but that will result in a “tool not found” error. And existing invalid server-side tool names will be already validated during tool registration on the server side - so additional validation seems doesn't add value.

[Kehrlann]

Proposal: using different validation options for different MCP servers registered in the same application is probably not a very typical use case. I would therefore propose a global setting to disable validation, with an option to override it per server in SyncSpecification, AsyncSpecification, StatelessAsyncSpecification, and StatelessSyncSpecification. WDYT? This keeps the default behavior simple while still allowing flexibility when integrating with non-compliant servers. What do you think?

I like the idea, something like .skipToolNameValidation(boolean skip)?

Therefore I am going to move validation into McpAsyncClient.listToolsInternal().

Works for me.

Do you think it makes sense to validate tool names on the server side during tool/call? Basically a client can call server with non-existent and invalid tool name, but that will result in a “tool not found” error. And existing invalid server-side tool names will be already validated during tool registration on the server side - so additional validation seems doesn't add value.

No, don't validate client calls on the server. If a tool name does not exist, it does not exist - no matter the reason.
If a tool is present but has an invalid name, that means the user configured their server with "skip validation" and therefore we assume they know what they are doing and do not want extra messages.