Add support for custom middleware in the correct order.#2026
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for passing custom middleware into NewMCPServer to enable the remote MCP server to inject metrics middleware that needs to execute after the default GitHub API error context middleware has set up the error context. The changes maintain backward compatibility by using a variadic parameter.
Changes:
- Added variadic middleware parameter to
NewMCPServerfunction signature - Reordered middleware application so error context middleware is outermost (added last, runs first)
- Added documentation comments explaining middleware execution order
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/github/server.go | Modified NewMCPServer to accept optional middleware parameter and reordered middleware application (custom→deps→errorContext) to ensure error context is set up before custom middleware executes |
| pkg/http/handler.go | Added comment to GitHubMCPServerFactoryFunc explaining that custom middleware executes after default middlewares |
Comments suppressed due to low confidence (1)
pkg/github/server.go:102
- The comment states the error context middleware should be "closest to the handler", but this is misleading. Since the error context middleware is applied last (line 105), it's actually the outermost middleware (farthest from the handler), not the closest. It runs first on the request path to set up the error context before other middleware and the handler execute. Consider revising to: "the error context middleware should be applied last so that it runs FIRST (outermost layer) to set up the error context before other middleware" for clarity.
// Add middlewares. Order matters - for example, the error context middleware should be applied last so that it runs FIRST (closest to the handler) to ensure all errors are captured,
// and any middleware that needs to read or modify the context should be before it.
| type MCPServerOption func(*mcp.ServerOptions) | ||
|
|
||
| func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependencies, inv *inventory.Inventory) (*mcp.Server, error) { | ||
| func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependencies, inv *inventory.Inventory, middleware ...mcp.Middleware) (*mcp.Server, error) { |
There was a problem hiding this comment.
Consider adding test coverage for the new middleware parameter to verify that custom middleware is executed in the correct order relative to the default middlewares (error context and deps injection). A test could verify that custom middleware can successfully read errors from the context that were set by tools/handlers, confirming the middleware chain works as intended.
This issue also appears on line 101 of the same file.
| type InventoryFactoryFunc func(r *http.Request) (*inventory.Inventory, error) | ||
|
|
||
| // GitHubMCPServerFactoryFunc is a function type for creating a new MCP Server instance. | ||
| // middleware are applied AFTER the default GitHub MCP Server middlewares (like error context injection) |
There was a problem hiding this comment.
The comment "middleware are applied AFTER the default GitHub MCP Server middlewares" is ambiguous. "Applied AFTER" could be interpreted as "added to the stack after" or "executed after". Based on the code, custom middleware is added first (line 103) but executes after the default middlewares have set up the context. Consider clarifying: "middleware will execute after the default middlewares (error context, deps injection) have set up the request context, allowing them to read errors and dependencies set by the defaults".
| // middleware are applied AFTER the default GitHub MCP Server middlewares (like error context injection) | |
| // Custom middleware created via this factory will execute after the default GitHub MCP Server | |
| // middlewares (such as error context and dependency injection) have populated the request context, | |
| // allowing the custom middleware to read errors and dependencies set by the defaults. |
Summary
Allows passing of middleware stack into
NewMCPServer.Why
In
modelcontextprotocol/go-sdk, when applying receiving middleware, they are applied right to left. For our remote MCP server, we need middleware to be applied after the error context in order to allow a metrics middleware to pull out the errors.This necessitates us passing in the middleware into the constructor because if we add new middleware after we have returned the server from the constructor, any new middleware added via
svr.AddReceivingMiddlewarewill be run BEFORE any existing middleware.What changed
NewMCPServerMCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goNote: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs