Skip to content

Commit 2ac77d6

Browse files
authored
Fix feature flag checker and add insiders mode support (#1920)
* wip * add insiders routes * remove static checker param and clean up * add tests for X-MCP-Features header parsing * fix extractToolNames
1 parent bbaa877 commit 2ac77d6

File tree

10 files changed

+160
-73
lines changed

10 files changed

+160
-73
lines changed

docs/remote-server.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,13 +121,15 @@ The Remote GitHub MCP server supports the following URL path patterns:
121121
- `/` - Default toolset (see ["default" toolset](../README.md#default-toolset))
122122
- `/readonly` - Default toolset in read-only mode
123123
- `/insiders` - Default toolset with insiders mode enabled
124-
- `/insiders/readonly` - Default toolset with insiders mode in read-only mode
124+
- `/readonly/insiders` - Default toolset in read-only mode with insiders mode enabled
125125
- `/x/all` - All available toolsets
126126
- `/x/all/readonly` - All available toolsets in read-only mode
127127
- `/x/all/insiders` - All available toolsets with insiders mode enabled
128+
- `/x/all/readonly/insiders` - All available toolsets in read-only mode with insiders mode enabled
128129
- `/x/{toolset}` - Single specific toolset
129130
- `/x/{toolset}/readonly` - Single specific toolset in read-only mode
130131
- `/x/{toolset}/insiders` - Single specific toolset with insiders mode enabled
132+
- `/x/{toolset}/readonly/insiders` - Single specific toolset in read-only mode with insiders mode enabled
131133

132134
Note: `{toolset}` can only be a single toolset, not a comma-separated list. To combine multiple toolsets, use the `X-MCP-Toolsets` header instead. Path modifiers like `/readonly` and `/insiders` can be combined with the `X-MCP-Insiders` or `X-MCP-Readonly` headers.
133135

internal/ghmcp/server.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,8 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se
135135
WithReadOnly(cfg.ReadOnly).
136136
WithToolsets(cfg.EnabledToolsets).
137137
WithTools(github.CleanTools(cfg.EnabledTools)).
138-
WithServerInstructions()
139-
// WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures))
138+
WithServerInstructions().
139+
WithFeatureChecker(featureChecker)
140140

141141
// Apply token scope filtering if scopes are known (for PAT filtering)
142142
if cfg.TokenScopes != nil {

pkg/context/request.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,3 +65,35 @@ func IsLockdownMode(ctx context.Context) bool {
6565
}
6666
return false
6767
}
68+
69+
// insidersCtxKey is a context key for insiders mode
70+
type insidersCtxKey struct{}
71+
72+
// WithInsidersMode adds insiders mode state to the context
73+
func WithInsidersMode(ctx context.Context, enabled bool) context.Context {
74+
return context.WithValue(ctx, insidersCtxKey{}, enabled)
75+
}
76+
77+
// IsInsidersMode retrieves the insiders mode state from the context
78+
func IsInsidersMode(ctx context.Context) bool {
79+
if enabled, ok := ctx.Value(insidersCtxKey{}).(bool); ok {
80+
return enabled
81+
}
82+
return false
83+
}
84+
85+
// headerFeaturesCtxKey is a context key for raw header feature flags
86+
type headerFeaturesCtxKey struct{}
87+
88+
// WithHeaderFeatures stores the raw feature flags from the X-MCP-Features header into context
89+
func WithHeaderFeatures(ctx context.Context, features []string) context.Context {
90+
return context.WithValue(ctx, headerFeaturesCtxKey{}, features)
91+
}
92+
93+
// GetHeaderFeatures retrieves the raw feature flags from context
94+
func GetHeaderFeatures(ctx context.Context) []string {
95+
if features, ok := ctx.Value(headerFeaturesCtxKey{}).([]string); ok {
96+
return features
97+
}
98+
return nil
99+
}

pkg/github/dependencies.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,6 @@ type RequestDeps struct {
248248
lockdownMode bool
249249
RepoAccessOpts []lockdown.RepoAccessOption
250250
T translations.TranslationHelperFunc
251-
Flags FeatureFlags
252251
ContentWindowSize int
253252

254253
// Feature flag checker for runtime checks
@@ -380,6 +379,7 @@ func (d *RequestDeps) GetT() translations.TranslationHelperFunc { return d.T }
380379
func (d *RequestDeps) GetFlags(ctx context.Context) FeatureFlags {
381380
return FeatureFlags{
382381
LockdownMode: d.lockdownMode && ghcontext.IsLockdownMode(ctx),
382+
InsidersMode: ghcontext.IsInsidersMode(ctx),
383383
}
384384
}
385385

pkg/http/features.go

Lines changed: 0 additions & 42 deletions
This file was deleted.

pkg/http/handler.go

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77

88
ghcontext "github.com/github/github-mcp-server/pkg/context"
99
"github.com/github/github-mcp-server/pkg/github"
10-
"github.com/github/github-mcp-server/pkg/http/headers"
1110
"github.com/github/github-mcp-server/pkg/http/middleware"
1211
"github.com/github/github-mcp-server/pkg/inventory"
1312
"github.com/github/github-mcp-server/pkg/translations"
@@ -31,6 +30,7 @@ type Handler struct {
3130
type HandlerOptions struct {
3231
GitHubMcpServerFactory GitHubMCPServerFactoryFunc
3332
InventoryFactory InventoryFactoryFunc
33+
FeatureChecker inventory.FeatureFlagChecker
3434
}
3535

3636
type HandlerOption func(*HandlerOptions)
@@ -47,6 +47,12 @@ func WithInventoryFactory(f InventoryFactoryFunc) HandlerOption {
4747
}
4848
}
4949

50+
func WithFeatureChecker(checker inventory.FeatureFlagChecker) HandlerOption {
51+
return func(o *HandlerOptions) {
52+
o.FeatureChecker = checker
53+
}
54+
}
55+
5056
func NewHTTPMcpHandler(
5157
ctx context.Context,
5258
cfg *ServerConfig,
@@ -66,7 +72,7 @@ func NewHTTPMcpHandler(
6672

6773
inventoryFactory := opts.InventoryFactory
6874
if inventoryFactory == nil {
69-
inventoryFactory = DefaultInventoryFactory(cfg, t, nil)
75+
inventoryFactory = DefaultInventoryFactory(cfg, t, opts.FeatureChecker)
7076
}
7177

7278
return &Handler{
@@ -85,11 +91,17 @@ func NewHTTPMcpHandler(
8591
func (h *Handler) RegisterRoutes(r chi.Router) {
8692
r.Use(middleware.WithRequestConfig)
8793

94+
// Base routes
8895
r.Mount("/", h)
89-
// Mount readonly and toolset routes
90-
r.With(withToolset).Mount("/x/{toolset}", h)
91-
r.With(withReadonly, withToolset).Mount("/x/{toolset}/readonly", h)
9296
r.With(withReadonly).Mount("/readonly", h)
97+
r.With(withInsiders).Mount("/insiders", h)
98+
r.With(withReadonly, withInsiders).Mount("/readonly/insiders", h)
99+
100+
// Toolset routes
101+
r.With(withToolset).Mount("/x/{toolset}", h)
102+
r.With(withToolset, withReadonly).Mount("/x/{toolset}/readonly", h)
103+
r.With(withToolset, withInsiders).Mount("/x/{toolset}/insiders", h)
104+
r.With(withToolset, withReadonly, withInsiders).Mount("/x/{toolset}/readonly/insiders", h)
93105
}
94106

95107
// withReadonly is middleware that sets readonly mode in the request context
@@ -109,6 +121,14 @@ func withToolset(next http.Handler) http.Handler {
109121
})
110122
}
111123

124+
// withInsiders is middleware that sets insiders mode in the request context
125+
func withInsiders(next http.Handler) http.Handler {
126+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
127+
ctx := ghcontext.WithInsidersMode(r.Context(), true)
128+
next.ServeHTTP(w, r.WithContext(ctx))
129+
})
130+
}
131+
112132
func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
113133
inventory, err := h.inventoryFactoryFunc(r)
114134
if err != nil {
@@ -141,15 +161,12 @@ func DefaultGitHubMCPServerFactory(r *http.Request, deps github.ToolDependencies
141161
return github.NewMCPServer(r.Context(), cfg, deps, inventory)
142162
}
143163

144-
func DefaultInventoryFactory(_ *ServerConfig, t translations.TranslationHelperFunc, staticChecker inventory.FeatureFlagChecker) InventoryFactoryFunc {
164+
// DefaultInventoryFactory creates the default inventory factory for HTTP mode
165+
func DefaultInventoryFactory(_ *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker) InventoryFactoryFunc {
145166
return func(r *http.Request) (*inventory.Inventory, error) {
146-
b := github.NewInventory(t).WithDeprecatedAliases(github.DeprecatedToolAliases)
147-
148-
// Feature checker composition
149-
headerFeatures := headers.ParseCommaSeparated(r.Header.Get(headers.MCPFeaturesHeader))
150-
if checker := ComposeFeatureChecker(headerFeatures, staticChecker); checker != nil {
151-
b = b.WithFeatureChecker(checker)
152-
}
167+
b := github.NewInventory(t).
168+
WithDeprecatedAliases(github.DeprecatedToolAliases).
169+
WithFeatureChecker(featureChecker)
153170

154171
b = InventoryFiltersForRequest(r, b)
155172
b.WithServerInstructions()

pkg/http/handler_test.go

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,13 @@ func mockTool(name, toolsetID string, readOnly bool) inventory.ServerTool {
3232
}
3333
}
3434

35+
func mockToolWithFeatureFlag(name, toolsetID string, readOnly bool, enableFlag, disableFlag string) inventory.ServerTool {
36+
tool := mockTool(name, toolsetID, readOnly)
37+
tool.FeatureFlagEnable = enableFlag
38+
tool.FeatureFlagDisable = disableFlag
39+
return tool
40+
}
41+
3542
func TestInventoryFiltersForRequest(t *testing.T) {
3643
tools := []inventory.ServerTool{
3744
mockTool("get_file_contents", "repos", true),
@@ -115,12 +122,15 @@ func testTools() []inventory.ServerTool {
115122
mockTool("create_issue", "issues", false),
116123
mockTool("list_pull_requests", "pull_requests", true),
117124
mockTool("create_pull_request", "pull_requests", false),
125+
// Feature-flagged tools for testing X-MCP-Features header
126+
mockToolWithFeatureFlag("needs_holdback", "repos", true, "mcp_holdback_consolidated_projects", ""),
127+
mockToolWithFeatureFlag("hidden_by_holdback", "repos", true, "", "mcp_holdback_consolidated_projects"),
118128
}
119129
}
120130

121131
// extractToolNames extracts tool names from an inventory
122-
func extractToolNames(inv *inventory.Inventory) []string {
123-
available := inv.AvailableTools(context.Background())
132+
func extractToolNames(ctx context.Context, inv *inventory.Inventory) []string {
133+
available := inv.AvailableTools(ctx)
124134
names := make([]string, len(available))
125135
for i, tool := range available {
126136
names[i] = tool.Tool.Name
@@ -141,17 +151,17 @@ func TestHTTPHandlerRoutes(t *testing.T) {
141151
{
142152
name: "root path returns all tools",
143153
path: "/",
144-
expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request"},
154+
expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "hidden_by_holdback"},
145155
},
146156
{
147157
name: "readonly path filters write tools",
148158
path: "/readonly",
149-
expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests"},
159+
expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "hidden_by_holdback"},
150160
},
151161
{
152162
name: "toolset path filters to toolset",
153163
path: "/x/repos",
154-
expectedTools: []string{"get_file_contents", "create_repository"},
164+
expectedTools: []string{"get_file_contents", "create_repository", "hidden_by_holdback"},
155165
},
156166
{
157167
name: "toolset path with issues",
@@ -161,7 +171,7 @@ func TestHTTPHandlerRoutes(t *testing.T) {
161171
{
162172
name: "toolset readonly path filters to readonly tools in toolset",
163173
path: "/x/repos/readonly",
164-
expectedTools: []string{"get_file_contents"},
174+
expectedTools: []string{"get_file_contents", "hidden_by_holdback"},
165175
},
166176
{
167177
name: "toolset readonly path with issues",
@@ -198,15 +208,15 @@ func TestHTTPHandlerRoutes(t *testing.T) {
198208
headers: map[string]string{
199209
headers.MCPReadOnlyHeader: "true",
200210
},
201-
expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests"},
211+
expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "hidden_by_holdback"},
202212
},
203213
{
204214
name: "X-MCP-Toolsets header filters to toolset",
205215
path: "/",
206216
headers: map[string]string{
207217
headers.MCPToolsetsHeader: "repos",
208218
},
209-
expectedTools: []string{"get_file_contents", "create_repository"},
219+
expectedTools: []string{"get_file_contents", "create_repository", "hidden_by_holdback"},
210220
},
211221
{
212222
name: "URL toolset takes precedence over header toolset",
@@ -222,19 +232,41 @@ func TestHTTPHandlerRoutes(t *testing.T) {
222232
headers: map[string]string{
223233
headers.MCPReadOnlyHeader: "false",
224234
},
225-
expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests"},
235+
expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "hidden_by_holdback"},
236+
},
237+
{
238+
name: "X-MCP-Features header enables flagged tool",
239+
path: "/",
240+
headers: map[string]string{
241+
headers.MCPFeaturesHeader: "mcp_holdback_consolidated_projects",
242+
},
243+
expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "needs_holdback"},
244+
},
245+
{
246+
name: "X-MCP-Features header with unknown flag is ignored",
247+
path: "/",
248+
headers: map[string]string{
249+
headers.MCPFeaturesHeader: "unknown_flag",
250+
},
251+
expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "hidden_by_holdback"},
226252
},
227253
}
228254

229255
for _, tt := range tests {
230256
t.Run(tt.name, func(t *testing.T) {
231257
var capturedInventory *inventory.Inventory
258+
var capturedCtx context.Context
259+
260+
// Create feature checker that reads from context (same as production)
261+
featureChecker := createHTTPFeatureChecker()
232262

233263
// Create inventory factory that captures the built inventory
234264
inventoryFactory := func(r *http.Request) (*inventory.Inventory, error) {
265+
capturedCtx = r.Context()
235266
builder := inventory.NewBuilder().
236267
SetTools(tools).
237-
WithToolsets([]string{"all"})
268+
WithToolsets([]string{"all"}).
269+
WithFeatureChecker(featureChecker)
238270
builder = InventoryFiltersForRequest(r, builder)
239271
inv, err := builder.Build()
240272
if err != nil {
@@ -277,7 +309,7 @@ func TestHTTPHandlerRoutes(t *testing.T) {
277309
// Verify the inventory was captured and has the expected tools
278310
require.NotNil(t, capturedInventory, "inventory should have been created")
279311

280-
toolNames := extractToolNames(capturedInventory)
312+
toolNames := extractToolNames(capturedCtx, capturedInventory)
281313
expectedSorted := make([]string, len(tt.expectedTools))
282314
copy(expectedSorted, tt.expectedTools)
283315
sort.Strings(expectedSorted)

pkg/http/headers/headers.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ const (
3434
MCPToolsHeader = "X-MCP-Tools"
3535
// MCPLockdownHeader indicates whether lockdown mode is enabled.
3636
MCPLockdownHeader = "X-MCP-Lockdown"
37+
// MCPInsidersHeader indicates whether insiders mode is enabled for early access features.
38+
MCPInsidersHeader = "X-MCP-Insiders"
3739
// MCPFeaturesHeader is a comma-separated list of feature flags to enable.
3840
MCPFeaturesHeader = "X-MCP-Features"
3941
)

0 commit comments

Comments
 (0)