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 <me@adamholt.co.uk>
* 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.
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 <175728472+Copilot@users.noreply.github.com> * 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 <175728472+Copilot@users.noreply.github.com> * 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 <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Holt <me@adamholt.co.uk>
* 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 <175728472+Copilot@users.noreply.github.com> * 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 <175728472+Copilot@users.noreply.github.com> * 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 <mattdholloway@pm.me> Co-authored-by: Matt Holloway <mattdholloway@github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
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.
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