-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add Streamable HTTP mode. #1849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Auto-generated by license-check workflow
* add readonly and toolset support * address feedback * forgotten files * remove redundant checks in WithRequestConfig * move middleware in RegisterRoutes * improve comment and add TestParseCommaSeparated * fix broken TestInventoryFiltersForRequest * parse X-MCP-Tools into ctx * clean up TestInventoryFiltersForRequest * Pass context to handler, but use request context for per-request data * Pass through the context in MCP server creation functions --------- Co-authored-by: Adam Holt <[email protected]>
* add readonly and toolset support * address feedback * forgotten files * remove redundant checks in WithRequestConfig * move middleware in RegisterRoutes * improve comment and add TestParseCommaSeparated * fix broken TestInventoryFiltersForRequest * parse X-MCP-Tools into ctx * clean up TestInventoryFiltersForRequest * review http args and add lockdown ctx helpers * parse lockdown header and update GetFlags to retrieve ff from ctx * clean up and fix tests * fix GetFlags check, move lockdown parsing in WithRequestConfig and fix broken tests
…r for the HTTP server.
42b7f64 to
97e8f35
Compare
SamMorrowDrums
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is going really, a couple of questions/comments/clarifications for now. Let me know if any specific things I should look deeper at sooner rather than later.
* wip * add insiders routes * remove static checker param and clean up * add tests for X-MCP-Features header parsing * fix extractToolNames
* initial oauth metadata implementation * add nolint for GetEffectiveHostAndScheme * remove CAPI reference * remove nonsensical example URL * anonymize * add oauth tests * replace custom protected resource metadata handler with our own * remove unused header * Update pkg/http/oauth/oauth.go Co-authored-by: Copilot <[email protected]> * pass oauth config to mcp handler for token extraction * chore: retrigger ci * align types with base branch * update more types * initial oauth metadata implementation * add nolint for GetEffectiveHostAndScheme * remove CAPI reference * remove nonsensical example URL * anonymize * add oauth tests * replace custom protected resource metadata handler with our own * Update pkg/http/oauth/oauth.go Co-authored-by: Copilot <[email protected]> * chore: retrigger ci * update more types * remove CAPI specific header * restore mcp path specific logic * implement better resource path handling for OAuth server * return auth handler to lib version * rename to base-path flag * switch to chi group * make viper commands http only * Default to http, but check for TLS in GetEffectiveHostAndScheme --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Adam Holt <[email protected]>
* initial oauth metadata implementation * add nolint for GetEffectiveHostAndScheme * remove CAPI reference * remove nonsensical example URL * anonymize * add oauth tests * replace custom protected resource metadata handler with our own * remove unused header * Update pkg/http/oauth/oauth.go Co-authored-by: Copilot <[email protected]> * pass oauth config to mcp handler for token extraction * chore: retrigger ci * align types with base branch * update more types * initial oauth metadata implementation * add nolint for GetEffectiveHostAndScheme * remove CAPI reference * remove nonsensical example URL * anonymize * add oauth tests * replace custom protected resource metadata handler with our own * Update pkg/http/oauth/oauth.go Co-authored-by: Copilot <[email protected]> * chore: retrigger ci * update more types * remove CAPI specific header * restore mcp path specific logic * WIP * implement better resource path handling for OAuth server * return auth handler to lib version * rename to base-path flag * Add scopes challenge middleware to HTTP handler and initialize global tool scope map * Flags on the http command * Add tests for scope maps * Add scope challenge & pat filtering based on token scopes * Add scope filtering if challenge is enabled * Linter fixes and renamed scope challenge to PAT scope filter * Linter issues. * Remove unsused methoodod FetchScopesFromGitHubAPI, store active scopes in context * Require an API host when creating scope fetchers * Add tests for MCP parsing middleware * Remove IDE token support, these will never work. Add tests. * Move ForMCPRequest call to HTTP handler & explicitly set capabilities --------- Co-authored-by: Matt Holloway <[email protected]> Co-authored-by: Matt Holloway <[email protected]> Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for Streamable HTTP mode to the GitHub MCP Server, enabling HTTP-based MCP requests in addition to the existing stdio mode. The changes include significant refactoring to support request-scoped dependencies and OAuth scope challenges.
Changes:
- Adds
httpsubcommand with HTTP server implementation supporting various path patterns - Refactors server/inventory creation into factory methods for reusability
- Introduces
RequestDepsfor per-request dependency injection with lazy client initialization - Implements OAuth scope challenge middleware and token parsing utilities
- Extracts parameter handling methods into dedicated
params.gofile
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/http/server.go |
HTTP server setup with OAuth and middleware registration |
pkg/http/handler.go |
Request handler with inventory filtering and MCP server creation |
pkg/http/middleware/*.go |
Token extraction, request config parsing, MCP parsing, and scope challenge |
pkg/http/oauth/oauth.go |
RFC 9728 OAuth protected resource metadata implementation |
pkg/context/*.go |
Context utilities for token info, request config, and MCP method info |
pkg/utils/*.go |
Token parsing and API host resolution utilities |
pkg/github/server.go |
Refactored to extract NewMCPServer with configurable dependencies |
pkg/github/dependencies.go |
New RequestDeps for per-request lazy client initialization |
pkg/github/params.go |
Extracted parameter parsing functions from server.go |
pkg/scopes/*.go |
Updated scope fetcher to use APIHostResolver interface |
cmd/github-mcp-server/main.go |
Added http subcommand with flags |
SamMorrowDrums
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-Merge Review: Add Streamable HTTP Mode
Reviewed the full PR using semantic diff for triage, then get_file_contents with symbol extraction for deep dives into ~20 key functions/types.
Architecture — Well Structured
The decomposition is clean:
pkg/github/server.goownsNewMCPServer— shared by both stdio and HTTP, withMCPServerConfig+MCPServerOptionproviding the extensibility seam. Good.pkg/http/handler.gouses functional options (HandlerOption), factory functions for both inventory and MCP server creation, making the handler fully pluggable for the remote server to override. This is the right pattern for a library used downstream.internal/ghmcp/server.go→NewStdioMCPServeris now a thin wrapper that creates clients, deps, inventory, and delegates togithub.NewMCPServer. Clean separation.
Middleware Chain — Correct Ordering
ExtractUserToken → WithRequestConfig → WithMCPParse → WithPATScopes → [WithScopeChallenge]
This is the right order: token first, then request config (toolsets/readonly from headers/path), then MCP body parsing, then PAT scope detection, then optional scope challenge. Each middleware only depends on context values set by predecessors.
Security Observations
-
Token parsing (
ParseAuthorizationHeader): Correctly handlesBearer(case-insensitive), matches known GitHub prefixes (ghp_,gho_,ghu_,ghs_,github_pat_), and falls back to old 40-char hex pattern. RejectsGitHub-Bearer(encrypted tokens). This is solid. -
Per-request client construction (
RequestDeps.GetClient/GetGQLClient): Clients are constructed fresh per-request from the token in context — no shared mutable state, no token leaking between requests. The REST client usesWithAuthToken()and the GraphQL client uses aBearerAuthTransport. Both correctly resolve URLs from theAPIHostResolver. -
Scope challenge (
WithScopeChallenge): Only activates for OAuth tokens ontools/callrequests. Returns 403 withWWW-Authenticateheader containing the required + existing scopes and resource metadata URL. The fallback body parsing (whenWithMCPParsedidn't run) correctly restoresr.Bodyviaio.NopCloser(bytes.NewReader(body)). -
GetRepoAccessCachecreates a newlockdown.GetInstanceper call — worth verifyingGetInstancehandles caching/deduplication internally, otherwise this could be expensive in lockdown mode.
Nits / Minor Observations
-
Handler.ServeHTTPcreates a newmcp.Serverper request viagithubMcpServerFactory. This is intentional for stateless mode but worth documenting — it means server-level state (like registered tools) is rebuilt on every request. TheStateless: trueoption onStreamableHTTPOptionsconfirms this is by design. -
Handlerstores actxfield from construction time. This is the app-level signal context, but passing it through the struct rather than deriving from the request context is a pattern that could cause confusion. Currently it's only used forNewMCPServercalls which seems fine. -
InventoryFiltersForRequesthardcodesfalsefor dynamic toolsets in HTTP mode — there's a comment explaining this, good. -
Error responses in
ServeHTTPreturn bare500with no body/logging on inventory or server creation failure. Consider at minimum logging the error for debuggability. -
NewHTTPMcpHandlertakes 7 positional args + variadic options — the positional arg count is high. Could consider a config struct, but the functional options pattern for the optional parts is good.
Overall
Well-executed extraction that correctly separates concerns between stdio and HTTP modes while preserving the library contract for the remote server. The security-sensitive paths (token handling, per-request isolation, scope challenges) are implemented correctly. The factory/option patterns provide the right extensibility seams.
Summary
Adds support for Streamable HTTP mode.
Why
Addresses https://github.com/github/copilot-mcp-core/issues/1125.
What changed
httpsubcommand to the core binary.githubpackage, to be used by both STDIO and HTTP mode.utilspackageGetRepoAccessCacheandGetFlagsmethods onToolDependenciesnow require acontext.Context.MCP impact
Tool changes are limited to new
ToolDependenciesinterface changes.Security / limits
This is mostly porting across our existing Remote HTTP implementation.
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