fix(mcp-apps): defer _meta.ui strip to per-request RegisterTools (#2446)

mattdholloway · Copilot · web-flow · commit 0e2fc3889664 · 2026-05-08T09:08:50.000-07:00
* fix(mcp-apps): defer _meta.ui strip to per-request RegisterTools

The MCP Apps `_meta.ui` strip lived in `Builder.Build()`, which calls
`checkFeatureFlag(context.Background())`. The HTTP feature checker
(`createHTTPFeatureChecker`) reads insiders mode from the request
context &mdash; a background context never has it set, so the FF reported
MCP Apps off and the strip ran eagerly at server startup. Per-request
inventory factories then served pre-stripped tools regardless of
whether the request actually arrived on the `/insiders` route.

Symptom: `github/github-mcp-server-remote` returns 0 tools with
`_meta.ui` over HTTP `/insiders`, despite the source unconditionally
setting it on `get_me`, `issue_write`, and `create_pull_request`.
VS Code only renders MCP App UIs because of its persistent tool cache
from earlier deploys. Reproducible locally with
`cmd/github-mcp-server http --insiders` plus a vanilla curl tools/list.

Fix: drop the strip from `Build()`. Apply it in `RegisterTools(ctx,&hellip;)`
where the per-request context is in scope and the HTTP feature checker
can correctly detect insiders mode (or the remote checker can correctly
read user identity for Statsig flag lookup).

The same root cause affects `github/github-mcp-server-remote` &mdash; its
`featureflags.NewComposedFeatureFlagChecker` reads
`requestctx.User(ctx)`, which background context lacks, so the
`remote_mcp_ui_apps` Statsig flag always returned false. The fix here
covers both downstreams since `RegisterTools` is the single entry
point for tool registration.

Stdio mode is unaffected: it uses a closure-captured insiders mode
flag (`createFeatureChecker`) that does not depend on context, and the
per-request strip in `RegisterTools` produces the same outcome.

Verified end-to-end against the deployed remote tool definitions:

  HTTP /insiders     &rarr; 3 tools with _meta.ui (was 0)
  HTTP /             &rarr; 0 tools with _meta.ui (correct)
  stdio --insiders   &rarr; 3 tools with _meta.ui (unchanged)
  stdio              &rarr; 0 tools with _meta.ui (correct)

Adds:
  - pkg/http: TestInsidersRoutePreservesUIMeta &mdash; pins the regression
  - pkg/inventory: updates the existing strip tests to use the new
    RegisterTools-as-strip-site contract via a captureRegisteredTools
    helper

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;

* style: gofmt handler_test.go and registry_test.go

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;

* lint: address revive (context-as-argument) and unused checkFeatureFlag

- Reorder captureRegisteredTools params to put context.Context first
- Remove dead Builder.checkFeatureFlag (was only called by Build's
  former MCP Apps strip, now done in RegisterTools via the Inventory
  receiver's checkFeatureFlag instead)

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;

---------

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;
diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go
@@ -825,3 +825,56 @@ func TestCrossOriginProtection(t *testing.T) {
 		})
 	}
 }
+
+// TestInsidersRoutePreservesUIMeta is a regression test for the bug where
+// _meta.ui was stripped from tools/list responses on the HTTP /insiders route.
+//
+// Before the fix:
+//   - buildStaticInventory called Build() on a builder configured with the
+//     HTTP feature checker (which reads insiders mode from the request ctx).
+//   - Build() invoked checkFeatureFlag(context.Background()) &mdash; bg ctx has no
+//     insiders mode, so the FF reported MCP Apps off, and stripMCPAppsMetadata
+//     ran eagerly against the static tool slice at server startup.
+//   - Per-request inventory factories then served pre-stripped tools regardless
+//     of whether the request actually came in via /insiders.
+//
+// After the fix:
+//   - Build() no longer touches MCP Apps metadata.
+//   - RegisterTools applies the strip per-request, using the request context
+//     where the HTTP feature checker correctly observes insiders mode.
+func TestInsidersRoutePreservesUIMeta(t *testing.T) {
+	const uiURI = "ui://test/widget"
+	uiTool := mockTool("with_ui", "repos", true)
+	uiTool.Tool.Meta = mcp.Meta{"ui": map[string]any{"resourceUri": uiURI}}
+
+	checker := createHTTPFeatureChecker()
+	build := func() *inventory.Inventory {
+		inv, err := inventory.NewBuilder().
+			SetTools([]inventory.ServerTool{uiTool}).
+			WithFeatureChecker(checker).
+			WithToolsets([]string{"all"}).
+			Build()
+		require.NoError(t, err)
+		return inv
+	}
+
+	// Simulate a /insiders request: ctx has insiders mode set.
+	insidersCtx := ghcontext.WithInsidersMode(context.Background(), true)
+
+	// AvailableTools no longer strips _meta.ui (post-fix), regardless of ctx.
+	// The strip lives in RegisterTools, gated on the per-request FF check.
+	insidersTools := build().AvailableTools(insidersCtx)
+	plainTools := build().AvailableTools(context.Background())
+
+	// On the /insiders path, the FF check returns true &rarr; no strip &rarr; _meta preserved.
+	enabled, _ := checker(insidersCtx, "remote_mcp_ui_apps")
+	require.True(t, enabled, "FF should be on for /insiders ctx")
+	require.Len(t, insidersTools, 1)
+	require.NotNil(t, insidersTools[0].Tool.Meta, "_meta should be present on /insiders")
+	require.Equal(t, uiURI, insidersTools[0].Tool.Meta["ui"].(map[string]any)["resourceUri"])
+
+	// On the non-insiders path, RegisterTools strips _meta.ui.
+	plainEnabled, _ := checker(context.Background(), "remote_mcp_ui_apps")
+	require.False(t, plainEnabled, "FF should be off for non-insiders ctx")
+	require.Len(t, plainTools, 1)
+}
diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go
@@ -190,19 +190,6 @@ func cleanTools(tools []string) []string {
 	return cleaned
 }
 
-// checkFeatureFlag checks a feature flag at build time using the builder's feature checker.
-// Returns false if no checker is configured or the flag is not enabled.
-func (b *Builder) checkFeatureFlag(flag string) bool {
-	if b.featureChecker == nil {
-		return false
-	}
-	enabled, err := b.featureChecker(context.Background(), flag)
-	if err != nil {
-		return false
-	}
-	return enabled
-}
-
 // Build creates the final Inventory with all configuration applied.
 // This processes toolset filtering, tool name resolution, and sets up
 // the inventory for use. The returned Inventory is ready for use with
@@ -214,13 +201,6 @@ func (b *Builder) checkFeatureFlag(flag string) bool {
 func (b *Builder) Build() (*Inventory, error) {
 	tools := b.tools
 
-	// 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)
-	}
-
 	r := &Inventory{
 		tools:             tools,
 		resourceTemplates: b.resourceTemplates,
diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go
@@ -170,8 +170,19 @@ func (r *Inventory) ToolsetDescriptions() map[ToolsetID]string {
 
 // RegisterTools registers all available tools with the server using the provided dependencies.
 // The context is used for feature flag evaluation.
+//
+// MCP Apps UI metadata (`_meta.ui`) is stripped from the registered tools
+// when the MCP Apps feature flag is not enabled for this request. The strip
+// happens here (rather than at Build() time) so the per-request context is
+// in scope &mdash; HTTP feature checkers that read insiders mode or user identity
+// from ctx would otherwise see context.Background() and falsely report the
+// flag off, even when the actual request arrived on the /insiders route.
 func (r *Inventory) RegisterTools(ctx context.Context, s *mcp.Server, deps any) {
-	for _, tool := range r.AvailableTools(ctx) {
+	tools := r.AvailableTools(ctx)
+	if !r.checkFeatureFlag(ctx, mcpAppsFeatureFlag) {
+		tools = stripMCPAppsMetadata(tools)
+	}
+	for _, tool := range tools {
 		tool.RegisterFunc(s, deps)
 	}
 }
diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go
@@ -1863,18 +1863,16 @@ func TestWithMCPApps_DisabledStripsUIMetadata(t *testing.T) {
 		"description": "kept",
 	})
 
-	// Default: MCP Apps is disabled - UI meta should be stripped
+	// Default: MCP Apps is disabled - UI meta should be stripped on registration.
 	reg := mustBuild(t, NewBuilder().SetTools([]ServerTool{toolWithUI}).WithToolsets([]string{"all"}))
-	available := reg.AvailableTools(context.Background())
+	registered := captureRegisteredTools(context.Background(), t, reg)
 
-	require.Len(t, available, 1)
-	// UI metadata should be stripped
-	if available[0].Tool.Meta["ui"] != nil {
+	require.Len(t, registered, 1)
+	if registered[0].Meta["ui"] != nil {
 		t.Errorf("Expected 'ui' meta to be stripped, but it was present")
 	}
-	// Other metadata should be preserved
-	if available[0].Tool.Meta["description"] != "kept" {
-		t.Errorf("Expected 'description' meta to be preserved, got %v", available[0].Tool.Meta["description"])
+	if registered[0].Meta["description"] != "kept" {
+		t.Errorf("Expected 'description' meta to be preserved, got %v", registered[0].Meta["description"])
 	}
 }
 
@@ -1947,20 +1945,18 @@ func TestWithMCPApps_ToolsWithoutUIMetaUnaffected(t *testing.T) {
 }
 
 func TestWithMCPApps_UIOnlyMetaBecomesNil(t *testing.T) {
-	// Tool with ONLY ui metadata - should become nil after stripping when MCP Apps is disabled
 	toolUIOnly := mockToolWithMeta("tool_ui_only", "toolset1", map[string]any{
 		"ui": map[string]any{"html": "<div>hello</div>"},
 	})
 
 	reg := mustBuild(t, NewBuilder().
 		SetTools([]ServerTool{toolUIOnly}).
 		WithToolsets([]string{"all"}))
-	available := reg.AvailableTools(context.Background())
+	registered := captureRegisteredTools(context.Background(), t, reg)
 
-	require.Len(t, available, 1)
-	// Meta should be nil since ui was the only key and MCP Apps is off by default
-	if available[0].Tool.Meta != nil {
-		t.Errorf("Expected Meta to be nil after stripping only key, got %v", available[0].Tool.Meta)
+	require.Len(t, registered, 1)
+	if registered[0].Meta != nil {
+		t.Errorf("Expected Meta to be nil after stripping only key, got %v", registered[0].Meta)
 	}
 }
 
@@ -2239,3 +2235,25 @@ func TestCreateExcludeToolsFilter(t *testing.T) {
 	require.NoError(t, err)
 	require.True(t, allowed, "allowed_tool should be included")
 }
+
+// captureRegisteredTools mirrors RegisterTools' per-request strip behavior so
+// tests can verify what the wire sees, without requiring tools to have real
+// handlers (RegisterTools panics on tools without HandlerFunc).
+func captureRegisteredTools(ctx context.Context, t *testing.T, reg *Inventory) []*mcp.Tool {
+	t.Helper()
+	tools := reg.AvailableTools(ctx)
+	out := make([]*mcp.Tool, 0, len(tools))
+	for i := range tools {
+		toolCopy := tools[i].Tool
+		out = append(out, &toolCopy)
+	}
+	if !reg.checkFeatureFlag(ctx, mcpAppsFeatureFlag) {
+		for _, tt := range out {
+			delete(tt.Meta, "ui")
+			if len(tt.Meta) == 0 {
+				tt.Meta = nil
+			}
+		}
+	}
+	return out
+}
