From 81bf567c712e2ef157bf0c54698c7316e34c8c6e Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 8 May 2026 16:20:22 +0100 Subject: [PATCH 1/3] fix(mcp-apps): defer _meta.ui strip to per-request RegisterTools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 — 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,…)` 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` — 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 → 3 tools with _meta.ui (was 0) HTTP / → 0 tools with _meta.ui (correct) stdio --insiders → 3 tools with _meta.ui (unchanged) stdio → 0 tools with _meta.ui (correct) Adds: - pkg/http: TestInsidersRoutePreservesUIMeta — 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 <223556219+Copilot@users.noreply.github.com> --- pkg/http/handler_test.go | 53 ++++++++++++++++++++++++ pkg/inventory/builder.go | 7 ---- pkg/inventory/registry.go | 13 +++++- pkg/inventory/registry_test.go | 76 +++++++++++++++++++++------------- 4 files changed, 112 insertions(+), 37 deletions(-) diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 002266ba15..40e44a3739 100644 --- 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()) — 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 → no strip → _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 index b9a0d8548b..8c2af809ee 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -214,13 +214,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 index e2cd3a9e67..a0bbc7a550 100644 --- 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 — 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 index e6c9e450cb..5e3d3d3979 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1858,24 +1858,22 @@ func mockToolWithMeta(name string, toolsetID string, meta map[string]any) Server } func TestWithMCPApps_DisabledStripsUIMetadata(t *testing.T) { - toolWithUI := mockToolWithMeta("tool_with_ui", "toolset1", map[string]any{ - "ui": map[string]any{"html": "
hello
"}, - "description": "kept", - }) +toolWithUI := mockToolWithMeta("tool_with_ui", "toolset1", map[string]any{ +"ui": map[string]any{"html": "
hello
"}, +"description": "kept", +}) - // Default: MCP Apps is disabled - UI meta should be stripped - reg := mustBuild(t, NewBuilder().SetTools([]ServerTool{toolWithUI}).WithToolsets([]string{"all"})) - available := reg.AvailableTools(context.Background()) +// Default: MCP Apps is disabled - UI meta should be stripped on registration. +reg := mustBuild(t, NewBuilder().SetTools([]ServerTool{toolWithUI}).WithToolsets([]string{"all"})) +registered := captureRegisteredTools(t, reg, context.Background()) - require.Len(t, available, 1) - // UI metadata should be stripped - if available[0].Tool.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"]) - } +require.Len(t, registered, 1) +if registered[0].Meta["ui"] != nil { +t.Errorf("Expected 'ui' meta to be stripped, but it was present") +} +if registered[0].Meta["description"] != "kept" { +t.Errorf("Expected 'description' meta to be preserved, got %v", registered[0].Meta["description"]) +} } func TestWithMCPApps_EnabledPreservesUIMetadata(t *testing.T) { @@ -1947,21 +1945,19 @@ 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": "
hello
"}, - }) +toolUIOnly := mockToolWithMeta("tool_ui_only", "toolset1", map[string]any{ +"ui": map[string]any{"html": "
hello
"}, +}) - reg := mustBuild(t, NewBuilder(). - SetTools([]ServerTool{toolUIOnly}). - WithToolsets([]string{"all"})) - available := reg.AvailableTools(context.Background()) +reg := mustBuild(t, NewBuilder(). +SetTools([]ServerTool{toolUIOnly}). +WithToolsets([]string{"all"})) +registered := captureRegisteredTools(t, reg, context.Background()) - 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) +} } func TestStripMetaKeys(t *testing.T) { @@ -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(t *testing.T, reg *Inventory, ctx context.Context) []*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 +} From db41ab4d91071e811fb2573a81654a0d6e7e502e Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 8 May 2026 16:24:12 +0100 Subject: [PATCH 2/3] style: gofmt handler_test.go and registry_test.go Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/http/handler_test.go | 66 +++++++++++++-------------- pkg/inventory/registry_test.go | 82 +++++++++++++++++----------------- 2 files changed, 74 insertions(+), 74 deletions(-) diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 40e44a3739..9887ff1f3b 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -843,38 +843,38 @@ func TestCrossOriginProtection(t *testing.T) { // - 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 -} + 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 → no strip → _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) + // 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 → no strip → _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/registry_test.go b/pkg/inventory/registry_test.go index 5e3d3d3979..936f765947 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1858,22 +1858,22 @@ func mockToolWithMeta(name string, toolsetID string, meta map[string]any) Server } func TestWithMCPApps_DisabledStripsUIMetadata(t *testing.T) { -toolWithUI := mockToolWithMeta("tool_with_ui", "toolset1", map[string]any{ -"ui": map[string]any{"html": "
hello
"}, -"description": "kept", -}) + toolWithUI := mockToolWithMeta("tool_with_ui", "toolset1", map[string]any{ + "ui": map[string]any{"html": "
hello
"}, + "description": "kept", + }) -// Default: MCP Apps is disabled - UI meta should be stripped on registration. -reg := mustBuild(t, NewBuilder().SetTools([]ServerTool{toolWithUI}).WithToolsets([]string{"all"})) -registered := captureRegisteredTools(t, reg, context.Background()) + // Default: MCP Apps is disabled - UI meta should be stripped on registration. + reg := mustBuild(t, NewBuilder().SetTools([]ServerTool{toolWithUI}).WithToolsets([]string{"all"})) + registered := captureRegisteredTools(t, reg, context.Background()) -require.Len(t, registered, 1) -if registered[0].Meta["ui"] != nil { -t.Errorf("Expected 'ui' meta to be stripped, but it was present") -} -if registered[0].Meta["description"] != "kept" { -t.Errorf("Expected 'description' meta to be preserved, got %v", registered[0].Meta["description"]) -} + require.Len(t, registered, 1) + if registered[0].Meta["ui"] != nil { + t.Errorf("Expected 'ui' meta to be stripped, but it was present") + } + if registered[0].Meta["description"] != "kept" { + t.Errorf("Expected 'description' meta to be preserved, got %v", registered[0].Meta["description"]) + } } func TestWithMCPApps_EnabledPreservesUIMetadata(t *testing.T) { @@ -1945,19 +1945,19 @@ func TestWithMCPApps_ToolsWithoutUIMetaUnaffected(t *testing.T) { } func TestWithMCPApps_UIOnlyMetaBecomesNil(t *testing.T) { -toolUIOnly := mockToolWithMeta("tool_ui_only", "toolset1", map[string]any{ -"ui": map[string]any{"html": "
hello
"}, -}) + toolUIOnly := mockToolWithMeta("tool_ui_only", "toolset1", map[string]any{ + "ui": map[string]any{"html": "
hello
"}, + }) -reg := mustBuild(t, NewBuilder(). -SetTools([]ServerTool{toolUIOnly}). -WithToolsets([]string{"all"})) -registered := captureRegisteredTools(t, reg, context.Background()) + reg := mustBuild(t, NewBuilder(). + SetTools([]ServerTool{toolUIOnly}). + WithToolsets([]string{"all"})) + registered := captureRegisteredTools(t, reg, context.Background()) -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) -} + 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) + } } func TestStripMetaKeys(t *testing.T) { @@ -2240,20 +2240,20 @@ func TestCreateExcludeToolsFilter(t *testing.T) { // tests can verify what the wire sees, without requiring tools to have real // handlers (RegisterTools panics on tools without HandlerFunc). func captureRegisteredTools(t *testing.T, reg *Inventory, ctx context.Context) []*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 + 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 } From 97d6d20517cf6ad6e511c165c0b53c96c0e56eac Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 8 May 2026 16:29:50 +0100 Subject: [PATCH 3/3] 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 <223556219+Copilot@users.noreply.github.com> --- pkg/inventory/builder.go | 13 ------------- pkg/inventory/registry_test.go | 6 +++--- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 8c2af809ee..d656359bb6 100644 --- 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 diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 936f765947..77c3bb57e5 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1865,7 +1865,7 @@ func TestWithMCPApps_DisabledStripsUIMetadata(t *testing.T) { // Default: MCP Apps is disabled - UI meta should be stripped on registration. reg := mustBuild(t, NewBuilder().SetTools([]ServerTool{toolWithUI}).WithToolsets([]string{"all"})) - registered := captureRegisteredTools(t, reg, context.Background()) + registered := captureRegisteredTools(context.Background(), t, reg) require.Len(t, registered, 1) if registered[0].Meta["ui"] != nil { @@ -1952,7 +1952,7 @@ func TestWithMCPApps_UIOnlyMetaBecomesNil(t *testing.T) { reg := mustBuild(t, NewBuilder(). SetTools([]ServerTool{toolUIOnly}). WithToolsets([]string{"all"})) - registered := captureRegisteredTools(t, reg, context.Background()) + registered := captureRegisteredTools(context.Background(), t, reg) require.Len(t, registered, 1) if registered[0].Meta != nil { @@ -2239,7 +2239,7 @@ func TestCreateExcludeToolsFilter(t *testing.T) { // 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(t *testing.T, reg *Inventory, ctx context.Context) []*mcp.Tool { +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))