Skip to content

Commit 0e2fc38

Browse files
mattdhollowayCopilot
mattdholloway
and
Copilot
authored
fix(mcp-apps): defer _meta.ui strip to per-request RegisterTools (#2446)
* 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 — 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> * style: gofmt handler_test.go and registry_test.go Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * 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> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2dab994 commit 0e2fc38

4 files changed

Lines changed: 97 additions & 35 deletions

File tree

‎pkg/http/handler_test.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -825,3 +825,56 @@ func TestCrossOriginProtection(t *testing.T) {
825825
})
826826
}
827827
}
828+
829+
// TestInsidersRoutePreservesUIMeta is a regression test for the bug where
830+
// _meta.ui was stripped from tools/list responses on the HTTP /insiders route.
831+
//
832+
// Before the fix:
833+
// - buildStaticInventory called Build() on a builder configured with the
834+
// HTTP feature checker (which reads insiders mode from the request ctx).
835+
// - Build() invoked checkFeatureFlag(context.Background()) — bg ctx has no
836+
// insiders mode, so the FF reported MCP Apps off, and stripMCPAppsMetadata
837+
// ran eagerly against the static tool slice at server startup.
838+
// - Per-request inventory factories then served pre-stripped tools regardless
839+
// of whether the request actually came in via /insiders.
840+
//
841+
// After the fix:
842+
// - Build() no longer touches MCP Apps metadata.
843+
// - RegisterTools applies the strip per-request, using the request context
844+
// where the HTTP feature checker correctly observes insiders mode.
845+
func TestInsidersRoutePreservesUIMeta(t *testing.T) {
846+
const uiURI = "ui://test/widget"
847+
uiTool := mockTool("with_ui", "repos", true)
848+
uiTool.Tool.Meta = mcp.Meta{"ui": map[string]any{"resourceUri": uiURI}}
849+
850+
checker := createHTTPFeatureChecker()
851+
build := func() *inventory.Inventory {
852+
inv, err := inventory.NewBuilder().
853+
SetTools([]inventory.ServerTool{uiTool}).
854+
WithFeatureChecker(checker).
855+
WithToolsets([]string{"all"}).
856+
Build()
857+
require.NoError(t, err)
858+
return inv
859+
}
860+
861+
// Simulate a /insiders request: ctx has insiders mode set.
862+
insidersCtx := ghcontext.WithInsidersMode(context.Background(), true)
863+
864+
// AvailableTools no longer strips _meta.ui (post-fix), regardless of ctx.
865+
// The strip lives in RegisterTools, gated on the per-request FF check.
866+
insidersTools := build().AvailableTools(insidersCtx)
867+
plainTools := build().AvailableTools(context.Background())
868+
869+
// On the /insiders path, the FF check returns true → no strip → _meta preserved.
870+
enabled, _ := checker(insidersCtx, "remote_mcp_ui_apps")
871+
require.True(t, enabled, "FF should be on for /insiders ctx")
872+
require.Len(t, insidersTools, 1)
873+
require.NotNil(t, insidersTools[0].Tool.Meta, "_meta should be present on /insiders")
874+
require.Equal(t, uiURI, insidersTools[0].Tool.Meta["ui"].(map[string]any)["resourceUri"])
875+
876+
// On the non-insiders path, RegisterTools strips _meta.ui.
877+
plainEnabled, _ := checker(context.Background(), "remote_mcp_ui_apps")
878+
require.False(t, plainEnabled, "FF should be off for non-insiders ctx")
879+
require.Len(t, plainTools, 1)
880+
}

‎pkg/inventory/builder.go‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -190,19 +190,6 @@ func cleanTools(tools []string) []string {
190190
return cleaned
191191
}
192192

193-
// checkFeatureFlag checks a feature flag at build time using the builder's feature checker.
194-
// Returns false if no checker is configured or the flag is not enabled.
195-
func (b *Builder) checkFeatureFlag(flag string) bool {
196-
if b.featureChecker == nil {
197-
return false
198-
}
199-
enabled, err := b.featureChecker(context.Background(), flag)
200-
if err != nil {
201-
return false
202-
}
203-
return enabled
204-
}
205-
206193
// Build creates the final Inventory with all configuration applied.
207194
// This processes toolset filtering, tool name resolution, and sets up
208195
// the inventory for use. The returned Inventory is ready for use with
@@ -214,13 +201,6 @@ func (b *Builder) checkFeatureFlag(flag string) bool {
214201
func (b *Builder) Build() (*Inventory, error) {
215202
tools := b.tools
216203

217-
// When MCP Apps feature flag is not enabled, strip UI metadata from tools
218-
// so clients won't attempt to load UI resources.
219-
// The feature checker is the single source of truth for flag evaluation.
220-
if !b.checkFeatureFlag(mcpAppsFeatureFlag) {
221-
tools = stripMCPAppsMetadata(tools)
222-
}
223-
224204
r := &Inventory{
225205
tools: tools,
226206
resourceTemplates: b.resourceTemplates,

‎pkg/inventory/registry.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,8 +170,19 @@ func (r *Inventory) ToolsetDescriptions() map[ToolsetID]string {
170170

171171
// RegisterTools registers all available tools with the server using the provided dependencies.
172172
// The context is used for feature flag evaluation.
173+
//
174+
// MCP Apps UI metadata (`_meta.ui`) is stripped from the registered tools
175+
// when the MCP Apps feature flag is not enabled for this request. The strip
176+
// happens here (rather than at Build() time) so the per-request context is
177+
// in scope — HTTP feature checkers that read insiders mode or user identity
178+
// from ctx would otherwise see context.Background() and falsely report the
179+
// flag off, even when the actual request arrived on the /insiders route.
173180
func (r *Inventory) RegisterTools(ctx context.Context, s *mcp.Server, deps any) {
174-
for _, tool := range r.AvailableTools(ctx) {
181+
tools := r.AvailableTools(ctx)
182+
if !r.checkFeatureFlag(ctx, mcpAppsFeatureFlag) {
183+
tools = stripMCPAppsMetadata(tools)
184+
}
185+
for _, tool := range tools {
175186
tool.RegisterFunc(s, deps)
176187
}
177188
}

‎pkg/inventory/registry_test.go‎

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1863,18 +1863,16 @@ func TestWithMCPApps_DisabledStripsUIMetadata(t *testing.T) {
18631863
"description": "kept",
18641864
})
18651865

1866-
// Default: MCP Apps is disabled - UI meta should be stripped
1866+
// Default: MCP Apps is disabled - UI meta should be stripped on registration.
18671867
reg := mustBuild(t, NewBuilder().SetTools([]ServerTool{toolWithUI}).WithToolsets([]string{"all"}))
1868-
available := reg.AvailableTools(context.Background())
1868+
registered := captureRegisteredTools(context.Background(), t, reg)
18691869

1870-
require.Len(t, available, 1)
1871-
// UI metadata should be stripped
1872-
if available[0].Tool.Meta["ui"] != nil {
1870+
require.Len(t, registered, 1)
1871+
if registered[0].Meta["ui"] != nil {
18731872
t.Errorf("Expected 'ui' meta to be stripped, but it was present")
18741873
}
1875-
// Other metadata should be preserved
1876-
if available[0].Tool.Meta["description"] != "kept" {
1877-
t.Errorf("Expected 'description' meta to be preserved, got %v", available[0].Tool.Meta["description"])
1874+
if registered[0].Meta["description"] != "kept" {
1875+
t.Errorf("Expected 'description' meta to be preserved, got %v", registered[0].Meta["description"])
18781876
}
18791877
}
18801878

@@ -1947,20 +1945,18 @@ func TestWithMCPApps_ToolsWithoutUIMetaUnaffected(t *testing.T) {
19471945
}
19481946

19491947
func TestWithMCPApps_UIOnlyMetaBecomesNil(t *testing.T) {
1950-
// Tool with ONLY ui metadata - should become nil after stripping when MCP Apps is disabled
19511948
toolUIOnly := mockToolWithMeta("tool_ui_only", "toolset1", map[string]any{
19521949
"ui": map[string]any{"html": "<div>hello</div>"},
19531950
})
19541951

19551952
reg := mustBuild(t, NewBuilder().
19561953
SetTools([]ServerTool{toolUIOnly}).
19571954
WithToolsets([]string{"all"}))
1958-
available := reg.AvailableTools(context.Background())
1955+
registered := captureRegisteredTools(context.Background(), t, reg)
19591956

1960-
require.Len(t, available, 1)
1961-
// Meta should be nil since ui was the only key and MCP Apps is off by default
1962-
if available[0].Tool.Meta != nil {
1963-
t.Errorf("Expected Meta to be nil after stripping only key, got %v", available[0].Tool.Meta)
1957+
require.Len(t, registered, 1)
1958+
if registered[0].Meta != nil {
1959+
t.Errorf("Expected Meta to be nil after stripping only key, got %v", registered[0].Meta)
19641960
}
19651961
}
19661962

@@ -2239,3 +2235,25 @@ func TestCreateExcludeToolsFilter(t *testing.T) {
22392235
require.NoError(t, err)
22402236
require.True(t, allowed, "allowed_tool should be included")
22412237
}
2238+
2239+
// captureRegisteredTools mirrors RegisterTools' per-request strip behavior so
2240+
// tests can verify what the wire sees, without requiring tools to have real
2241+
// handlers (RegisterTools panics on tools without HandlerFunc).
2242+
func captureRegisteredTools(ctx context.Context, t *testing.T, reg *Inventory) []*mcp.Tool {
2243+
t.Helper()
2244+
tools := reg.AvailableTools(ctx)
2245+
out := make([]*mcp.Tool, 0, len(tools))
2246+
for i := range tools {
2247+
toolCopy := tools[i].Tool
2248+
out = append(out, &toolCopy)
2249+
}
2250+
if !reg.checkFeatureFlag(ctx, mcpAppsFeatureFlag) {
2251+
for _, tt := range out {
2252+
delete(tt.Meta, "ui")
2253+
if len(tt.Meta) == 0 {
2254+
tt.Meta = nil
2255+
}
2256+
}
2257+
}
2258+
return out
2259+
}

0 commit comments

Comments
 (0)