Skip to content

Conversation

@omgitsads
Copy link
Member

@omgitsads omgitsads commented Jan 20, 2026

Summary

Adds support for Streamable HTTP mode.

Why

Addresses https://github.com/github/copilot-mcp-core/issues/1125.

What changed

  • Adds a http subcommand to the core binary.
  • Moves Param methods into their own file
  • Extract inventory creation out to a factory method, so that we can provide our own implementation in our remote server
  • Extract server creation to a factory method, so that we can provide our own implementation in remote.
  • Consolidate MCP Server creation in the github package, to be used by both STDIO and HTTP mode.
  • Decoupled Inventory & Dependency creation from MCP Server creation, to allow custom implementations
  • Moved API URL building out to the utils package
  • GetRepoAccessCache and GetFlags methods on ToolDependencies now require a context.Context.
  • Implemented a request based dependency
  • Added Scope Challenge middleware for progressive token scope escalation.

MCP impact

  • No tool or API changes
  • Tool schema or behavior changed
  • New tool added

Tool changes are limited to new ToolDependencies interface changes.

Security / limits

  • No security or limits impact
  • Auth / permissions considered
  • Data exposure, filtering, or token/size limits considered

This is mostly porting across our existing Remote HTTP implementation.

Tool renaming

  • I am renaming tools as part of this PR (e.g. a part of a consolidation effort)
    • I have added the new tool aliases in deprecated_tool_aliases.go
  • I am not renaming tools as part of this PR

Note: 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

  • Linted locally with ./script/lint
  • Tested locally with ./script/test

Docs

  • Not needed
  • Updated (README / docs / examples)

@omgitsads omgitsads changed the title Add HTTP mode. [WIP] Add HTTP mode. Jan 20, 2026
omgitsads and others added 8 commits January 23, 2026 12:17
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
Copy link
Collaborator

@SamMorrowDrums SamMorrowDrums left a 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.

kerobbi and others added 2 commits January 29, 2026 17:37
* wip

* add insiders routes

* remove static checker param and clean up

* add tests for X-MCP-Features header parsing

* fix extractToolNames
mattdholloway and others added 7 commits January 30, 2026 12:51
* 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]>
@omgitsads omgitsads marked this pull request as ready for review February 5, 2026 15:31
@omgitsads omgitsads requested a review from a team as a code owner February 5, 2026 15:31
Copilot AI review requested due to automatic review settings February 5, 2026 15:31
Copy link
Contributor

Copilot AI left a 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 http subcommand with HTTP server implementation supporting various path patterns
  • Refactors server/inventory creation into factory methods for reusability
  • Introduces RequestDeps for per-request dependency injection with lazy client initialization
  • Implements OAuth scope challenge middleware and token parsing utilities
  • Extracts parameter handling methods into dedicated params.go file

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

@omgitsads omgitsads changed the title [WIP] Add HTTP mode. Add Streamable HTTP mode. Feb 6, 2026
@omgitsads omgitsads merged commit aa30220 into main Feb 6, 2026
16 checks passed
@omgitsads omgitsads deleted the http-stack-2 branch February 6, 2026 14:33
Copy link
Collaborator

@SamMorrowDrums SamMorrowDrums left a 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.go owns NewMCPServer — shared by both stdio and HTTP, with MCPServerConfig + MCPServerOption providing the extensibility seam. Good.
  • pkg/http/handler.go uses 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.goNewStdioMCPServer is now a thin wrapper that creates clients, deps, inventory, and delegates to github.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

  1. Token parsing (ParseAuthorizationHeader): Correctly handles Bearer (case-insensitive), matches known GitHub prefixes (ghp_, gho_, ghu_, ghs_, github_pat_), and falls back to old 40-char hex pattern. Rejects GitHub-Bearer (encrypted tokens). This is solid.

  2. 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 uses WithAuthToken() and the GraphQL client uses a BearerAuthTransport. Both correctly resolve URLs from the APIHostResolver.

  3. Scope challenge (WithScopeChallenge): Only activates for OAuth tokens on tools/call requests. Returns 403 with WWW-Authenticate header containing the required + existing scopes and resource metadata URL. The fallback body parsing (when WithMCPParse didn't run) correctly restores r.Body via io.NopCloser(bytes.NewReader(body)).

  4. GetRepoAccessCache creates a new lockdown.GetInstance per call — worth verifying GetInstance handles caching/deduplication internally, otherwise this could be expensive in lockdown mode.

Nits / Minor Observations

  • Handler.ServeHTTP creates a new mcp.Server per request via githubMcpServerFactory. This is intentional for stateless mode but worth documenting — it means server-level state (like registered tools) is rebuilt on every request. The Stateless: true option on StreamableHTTPOptions confirms this is by design.

  • Handler stores a ctx field 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 for NewMCPServer calls which seems fine.

  • InventoryFiltersForRequest hardcodes false for dynamic toolsets in HTTP mode — there's a comment explaining this, good.

  • Error responses in ServeHTTP return bare 500 with no body/logging on inventory or server creation failure. Consider at minimum logging the error for debuggability.

  • NewHTTPMcpHandler takes 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants