Feat compatible config merge for stdio mcp servers#37
Conversation
Reviewer's GuideThis PR refactors configuration merging to produce separate HTTP, stdio, and SSE configs, extends the core server to dispatch requests based on protocol type (with new fetch/invoke helpers for HTTP and stdio backends), introduces a stdio‐based MCP client transport and wrapper APIs, overhauls the pkg/mcp API types (JSON‐RPC, tools, capabilities), and applies minor fixes (ID field normalization, MIMEType renames, dependency updates). File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @LeoLiuYan - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| var params mcp.InitializeRequestParams | ||
| if err := json.Unmarshal(req.Params, ¶ms); err != nil { | ||
| s.sendProtocolError(c, req.Id, "Invalid initialize parameters", http.StatusBadRequest, mcp.ErrorCodeInvalidParams) | ||
| if err := json.Unmarshal(req.Params.([]byte), ¶ms); err != nil { |
There was a problem hiding this comment.
issue (bug_risk): Type assertion on req.Params may panic if not []byte.
Use a type switch or explicit check to avoid panics, or ensure callers always supply req.Params as []byte.
|
|
||
| // sendSuccessResponse sends a successful response | ||
| func (s *Server) sendSuccessResponse(c *gin.Context, conn session.Connection, req mcp.JSONRPCRequest, result any, isSSE bool) { | ||
| resultBytes, _ := json.Marshal(result) |
There was a problem hiding this comment.
issue (bug_risk): Ignoring errors from json.Marshal may hide serialization issues.
Handle the error returned by json.Marshal instead of ignoring it. Return a protocol error if marshaling fails to avoid silent failures.
| var params mcp.InitializeRequestParams | ||
| if err := json.Unmarshal(req.Params, ¶ms); err != nil { | ||
| s.sendProtocolError(c, req.Id, fmt.Sprintf("invalid initialize parameters: %v", err), http.StatusBadRequest, mcp.ErrorCodeInvalidParams) | ||
| if err := json.Unmarshal(req.Params.([]byte), ¶ms); err != nil { |
There was a problem hiding this comment.
issue (bug_risk): Type assertion on req.Params to []byte may panic.
Use a type switch or ensure req.Params is a []byte before unmarshaling to avoid panics.
| newState.prefixToStdioServerConfig[prefix] = &cfg.StdioServer | ||
| newState.prefixToSSEServerConfig[prefix] = &cfg.SSEServer |
There was a problem hiding this comment.
issue (bug_risk): Storing pointers to config fields that may be overwritten in the next iteration.
Copy the StdioServer and SSEServer structs (or store their values) instead of taking pointers, so each prefix retains its own config rather than all referencing the final loop variable.
| } | ||
|
|
||
| return mergedConfig, nil | ||
| return []*config.MCPConfig{mergedHTTPConfig, mergedStdioConfig, mergedSSEConfig}, nil |
There was a problem hiding this comment.
suggestion: Returning all protocol configs even if empty may cause confusion downstream.
Filter out empty configs and return only those with merged data.
| } | ||
|
|
||
| func mergeConfigStdio(base, override config.StdioServerConfig) config.StdioServerConfig { | ||
| return override |
There was a problem hiding this comment.
suggestion (bug_risk): Merging Stdio/SSE config by overwriting may lose data from base config.
These functions discard the base config by returning only the override. For partial merges, implement a field-by-field merge to avoid data loss.
Suggested implementation:
func mergeConfigStdio(base, override config.StdioServerConfig) config.StdioServerConfig {
// Start from the base config…
merged := base
// …then selectively override only the non-default override values
// Enabled flag: override only if explicitly true
if override.Enabled {
merged.Enabled = true
}
// Address (or Host): override if provided
if override.Address != "" {
merged.Address = override.Address
}
// Port: override if non-zero
if override.Port != 0 {
merged.Port = override.Port
}
// TLS settings: override if non-nil
if override.TLS != nil {
merged.TLS = override.TLS
}
// (Repeat for any other StdioServerConfig fields, e.g. timeouts, buffer sizes, etc.)
return merged
}Make sure the above fields (Enabled, Address, Port, TLS, etc.) match those in your actual config.StdioServerConfig. For every field in that struct, add a corresponding “if override.FIELD is non-default, merged.FIELD = override.FIELD” clause.
2 participants
Summary by Sourcery
Enable full support for stdio-based MCP servers alongside existing HTTP and SSE protocols by expanding configuration schemas, merging logic, and server dispatch to handle multiple protocol types uniformly.
New Features:
Enhancements: