mcp: HTTP Header Standardization for x-mcp-header#915
mcp: HTTP Header Standardization for x-mcp-header#915guglielmo-san wants to merge 24 commits intomainfrom
Conversation
…ard request headers
…treamableClientConn
…rs for tool calls
…e comparison for parameter validation
…es and add structural validation
…ttp_standardization_2 # Conflicts: # mcp/streamable.go # mcp/streamable_client_test.go # mcp/streamable_headers.go # mcp/streamable_headers_test.go # mcp/streamable_test.go
…ernal JSON unmarshaling
…ors in ClientSession
…treamable_test.go
…_http_standardization_2' into guglielmoc/SEP-2243_http_standardization_2
guglielmo-san
changed the title
| // Avoid sending nil over the wire. | ||
| params.Arguments = map[string]any{} | ||
| } | ||
| if tool := cs.getCachedTool(params.Name); tool != nil { |
There was a problem hiding this comment.
Please also think how modelcontextprotocol/modelcontextprotocol#2549 will factor in into this solution.
There was a problem hiding this comment.
WDYT about merging this with streamable_headers.go? I think we already have a lot of files and the existing one is still not that big. Unless there are some particular reasons to separate it?
There was a problem hiding this comment.
In my opinion from a logic point of view they should be divided into two different files, but I can agree that we already have a lot of different files...
| } | ||
| if msg.Method == "tools/call" { | ||
| if tool, ok := ctx.Value(toolContextKey).(*Tool); ok { | ||
| msg.Extra = tool |
There was a problem hiding this comment.
Maybe just pass the context to setStandardHeaders to keep the logic using a single mechanism?
There was a problem hiding this comment.
I did not want to pass the ctx to function, but I agree that simplify the implementation
| http.Error(w, "failed connection", http.StatusInternalServerError) | ||
| return | ||
| } | ||
| transport.connection.toolLookup = func(name string) *Tool { |
There was a problem hiding this comment.
Does this need to be delayed with regard to transport creation in line 407?
There was a problem hiding this comment.
Yes, the transport.connection is created only in the server.Connect(). Unless we duplicate the toolLookup both in transport and connection
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result.Tools = filterValidTools(result.Tools) |
There was a problem hiding this comment.
In general this proposal adds HTTP specific functionality, so I'm a bit surprised that the spec requires general filtering. IMO ideally this change should be contained within streamable transport as much as possible. I'll ask on the SEP for clarification.
maciej-kisiel
left a comment
maciej-kisiel
left a comment
There was a problem hiding this comment.
Also some additional comments from AI review :)
…er validation to support JSON schema annotations
2 participants
Description
Implements SEP-2243 (HTTP Header Standardization) for x-mcp-param custom header.
Fixes #905