Migrate MCP Apps support from insiders mode to feature flag with insiders opt-in#2335
Migrate MCP Apps support from insiders mode to feature flag with insiders opt-in#2335SamMorrowDrums wants to merge 2 commits intomainfrom
Conversation
Rebase PR #2282 onto main (post-#2332) and unify feature flag allowlists into a single source of truth. - Add MCPAppsFeatureFlag, AllowedFeatureFlags, InsidersFeatureFlags, and ResolveFeatureFlags in feature_flags.go - AllowedFeatureFlags includes all user-controllable flags (MCP Apps + granular), InsidersFeatureFlags only includes MCPAppsFeatureFlag - HeaderAllowedFeatureFlags() now delegates to AllowedFeatureFlags - Builder uses feature checker instead of insidersMode bool - Remove InsidersOnly field from ServerTool and WithInsidersMode from Builder - HTTP feature checker uses ResolveFeatureFlags for per-request resolution with insiders expansion - Tool handlers check MCPAppsFeatureFlag via IsFeatureEnabled instead of InsidersMode Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates MCP Apps enablement from a hardcoded “insiders mode” gate to a centralized feature-flag model (remote_mcp_ui_apps), and unifies the allowlists used across HTTP headers and CLI/server behavior.
Changes:
- Centralizes user-controllable feature flags and insiders-mode expansion via
AllowedFeatureFlags,InsidersFeatureFlags, andResolveFeatureFlags(). - Refactors inventory building/metadata stripping to use feature-flag checks instead of a dedicated insiders-mode builder option.
- Updates HTTP and stdio servers to use the centralized feature resolution, and updates docs/tests accordingly.
Show a summary per file
| File | Description |
|---|---|
| pkg/inventory/server_tool.go | Removes InsidersOnly tool-level gating from the inventory model. |
| pkg/inventory/builder.go | Replaces WithInsidersMode with MCP Apps feature-flag based UI metadata stripping helpers. |
| pkg/inventory/registry_test.go | Updates tests to reflect MCP Apps feature-flag behavior and metadata stripping helpers. |
| pkg/http/server.go | Switches HTTP feature checking to use centralized ResolveFeatureFlags() per request. |
| pkg/http/server_test.go | Updates tests for feature flag behavior, including MCP Apps and insiders expansion. |
| pkg/http/handler.go | Removes static WithInsidersMode usage; relies on feature checker + builder behavior. |
| pkg/http/handler_test.go | Updates static inventory construction to no longer pass insiders mode into the builder. |
| pkg/github/feature_flags.go | Introduces centralized allowlist + insiders expansion + ResolveFeatureFlags(). |
| pkg/github/feature_flags_test.go | Adds tests for ResolveFeatureFlags() behavior (filtering/dedup/insiders). |
| pkg/github/tools.go | Makes HeaderAllowedFeatureFlags() delegate to the centralized allowlist. |
| internal/ghmcp/server.go | Uses centralized feature resolution for stdio server and MCP Apps UI resource registration. |
| docs/server-configuration.md | Documents MCP Apps as a feature flag and adds feature flags to the config matrix. |
Copilot's findings
- Files reviewed: 12/12 changed files
- Comments generated: 3
| // When MCP Apps feature flag is not enabled, strip UI metadata from tools | ||
| // so clients won't attempt to load UI resources. | ||
| // The feature checker is the single source of truth for flag evaluation. | ||
| if !b.checkFeatureFlag(mcpAppsFeatureFlag) { | ||
| tools = stripMCPAppsMetadata(tools) | ||
| } | ||
|
|
There was a problem hiding this comment.
Builder.Build() strips MCP Apps UI metadata based on b.checkFeatureFlag(), but that helper calls the feature checker with context.Background(). In HTTP mode the feature checker derives flags from request context (X-MCP-Features / insiders), so this build-time check will always evaluate as disabled and permanently strip UI metadata even when the request enables remote_mcp_ui_apps. To support per-request feature flags, move the UI metadata stripping to runtime (e.g., in Inventory.AvailableTools using the provided ctx), or change the builder API so Build can accept/propagate the request context when evaluating feature flags.
| // When MCP Apps feature flag is not enabled, strip UI metadata from tools | |
| // so clients won't attempt to load UI resources. | |
| // The feature checker is the single source of truth for flag evaluation. | |
| if !b.checkFeatureFlag(mcpAppsFeatureFlag) { | |
| tools = stripMCPAppsMetadata(tools) | |
| } |
| // AllowedFeatureFlags is the allowlist of feature flags that can be enabled | ||
| // by users via --features CLI flag or X-MCP-Features HTTP header. | ||
| // Only flags in this list are accepted; unknown flags are silently ignored. | ||
| // This is the single source of truth for which flags are user-controllable. | ||
| var AllowedFeatureFlags = []string{ | ||
| MCPAppsFeatureFlag, | ||
| FeatureFlagIssuesGranular, | ||
| FeatureFlagPullRequestsGranular, | ||
| } |
There was a problem hiding this comment.
AllowedFeatureFlags is exported as a mutable slice variable. External callers can accidentally append/modify it, which can lead to hard-to-debug behavior changes and potential data races in concurrent programs. Consider making the underlying allowlist unexported and exposing an exported function that returns a cloned slice (similar to HeaderAllowedFeatureFlags), or otherwise ensuring callers cannot mutate the shared backing array.
| // createHTTPFeatureChecker creates a feature checker that resolves features | ||
| // per-request by reading header features and insiders mode from context, | ||
| // then validating against the centralized AllowedFeatureFlags allowlist. | ||
| func createHTTPFeatureChecker() inventory.FeatureFlagChecker { | ||
| // Pre-compute whitelist as set for O(1) lookup | ||
| knownSet := make(map[string]bool, len(knownFeatureFlags)) | ||
| for _, f := range knownFeatureFlags { | ||
| knownSet[f] = true | ||
| } | ||
|
|
||
| return func(ctx context.Context, flag string) (bool, error) { | ||
| if knownSet[flag] && slices.Contains(ghcontext.GetHeaderFeatures(ctx), flag) { | ||
| return true, nil | ||
| } | ||
| return false, nil | ||
| headerFeatures := ghcontext.GetHeaderFeatures(ctx) | ||
| insidersMode := ghcontext.IsInsidersMode(ctx) | ||
| effective := github.ResolveFeatureFlags(headerFeatures, insidersMode) | ||
| return effective[flag], nil | ||
| } |
There was a problem hiding this comment.
createHTTPFeatureChecker calls ResolveFeatureFlags on every feature check, rebuilding maps each time. Inventory filtering can invoke the checker many times per request (once per feature-flagged tool), so this adds avoidable per-request overhead. Consider computing the effective feature set once per request (e.g., in middleware when parsing headers, or by caching within the checker using a request-scoped value) and doing O(1) lookups from that cached set.
Summary
Rebases #2282 onto main (post-#2332) and unifies the feature flag allowlists into a single source of truth. MCP Apps is now controlled by the
remote_mcp_ui_appsfeature flag rather than a hardcoded insiders mode check.Why
Closes https://github.com/github/copilot-mcp-core/issues/1471
What changed
Feature flag centralization (
pkg/github/feature_flags.go):AllowedFeatureFlags— single source of truth for all user-controllable flags (MCPApps + granular)InsidersFeatureFlags— onlyMCPAppsFeatureFlag(granular flags are NOT insiders features)ResolveFeatureFlags()— resolves explicit features + insiders expansion against the allowlistMCPAppsFeatureFlagconstantUnified allowlist (
pkg/github/tools.go):HeaderAllowedFeatureFlags()now delegates toAllowedFeatureFlagsinstead of maintaining a separate listBuilder refactor (
pkg/inventory/builder.go):WithInsidersMode()— replaced by feature checkerBuild()usescheckFeatureFlag(mcpAppsFeatureFlag)to decide UI metadata strippingstripInsidersFeatures→stripMCPAppsMetadata,stripInsidersMetaFromTool→stripMetaKeysServer changes:
internal/ghmcp/server.go:createFeatureCheckernow takesinsidersModeand usesResolveFeatureFlagspkg/http/server.go:createHTTPFeatureCheckerusesResolveFeatureFlagsper-requestpkg/http/handler.go: RemovedWithInsidersModecalls from inventory buildersTool changes:
issues.go,pullrequests.go: CheckMCPAppsFeatureFlagviaIsFeatureEnabledinstead ofInsidersModeInsidersOnlyfield fromServerToolExported API for remote server:
AllowedFeatureFlags,HeaderAllowedFeatureFlags(),MCPAppsFeatureFlag,ResolveFeatureFlags()are all exportedHeaderAllowedFeatureFlags()as source of truth instead of hardcoding its own allowlistMCP impact
Lint & tests
./script/lint./script/testDocs