Skip to content

Commit 9c25c02

Browse files
refactor: generic toolset+name sort, clarify feature flag intent
Address review feedback on #2450: - Collapse the three near-identical sort helpers in pkg/inventory/filters.go into a generic sortByToolsetThenName so adding new inventory item types doesn't require copying the comparator. - Expand the doc comments on the three *WithoutFeatureFiltering helpers to spell out why they exist: HTTP mode builds a static (process-wide) inventory as an upper bound, but per-request feature flags from headers (X-MCP-Features, X-MCP-Insiders) are evaluated later, so feature-flagged variants must be preserved here. - Strengthen the doc comment on ResolveFeatureFlags to make the contract explicit: user-supplied flags are validated against AllowedFeatureFlags, but insiders expansion deliberately is not — InsidersFeatureFlags may include server-controlled flags that are not user-toggleable. CORS comments are intentionally left for the PR author. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent bfa6fee commit 9c25c02

2 files changed

Lines changed: 44 additions & 22 deletions

File tree

pkg/github/feature_flags.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,13 @@ type FeatureFlags struct {
3838
}
3939

4040
// ResolveFeatureFlags computes the effective set of enabled feature flags by:
41-
// 1. Taking explicitly enabled features validated against AllowedFeatureFlags
42-
// 2. Adding features enabled by insiders mode from InsidersFeatureFlags
41+
// 1. Taking the user-supplied flags (from --features or X-MCP-Features) and
42+
// keeping only those present in AllowedFeatureFlags. Unknown or unsafe
43+
// flags from request input are silently dropped here.
44+
// 2. If insiders mode is on, unioning in every flag from InsidersFeatureFlags.
45+
// Insiders is a server-controlled meta switch, so its expansion is NOT
46+
// re-validated against AllowedFeatureFlags — InsidersFeatureFlags may
47+
// intentionally include flags that are not user-controllable.
4348
//
4449
// Returns a set (map) for O(1) lookup by the feature checker.
4550
func ResolveFeatureFlags(enabledFeatures []string, insidersMode bool) map[string]bool {

pkg/inventory/filters.go

Lines changed: 37 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -103,15 +103,26 @@ func (r *Inventory) isToolEnabledWithFeatureFlags(ctx context.Context, tool *Ser
103103
return true
104104
}
105105

106-
func sortTools(tools []ServerTool) {
107-
sort.Slice(tools, func(i, j int) bool {
108-
if tools[i].Toolset.ID != tools[j].Toolset.ID {
109-
return tools[i].Toolset.ID < tools[j].Toolset.ID
106+
// sortByToolsetThenName sorts items deterministically by their toolset ID,
107+
// breaking ties by name. The two extractor closures keep this generic helper
108+
// independent of the concrete inventory item shape (tools, resource templates,
109+
// prompts).
110+
func sortByToolsetThenName[T any](items []T, toolsetID func(T) ToolsetID, name func(T) string) {
111+
sort.Slice(items, func(i, j int) bool {
112+
if toolsetID(items[i]) != toolsetID(items[j]) {
113+
return toolsetID(items[i]) < toolsetID(items[j])
110114
}
111-
return tools[i].Tool.Name < tools[j].Tool.Name
115+
return name(items[i]) < name(items[j])
112116
})
113117
}
114118

119+
func sortTools(tools []ServerTool) {
120+
sortByToolsetThenName(tools,
121+
func(t ServerTool) ToolsetID { return t.Toolset.ID },
122+
func(t ServerTool) string { return t.Tool.Name },
123+
)
124+
}
125+
115126
// AvailableTools returns the tools that pass all current filters,
116127
// sorted deterministically by toolset ID, then tool name.
117128
// The context is used for feature flag evaluation.
@@ -132,6 +143,12 @@ func (r *Inventory) AvailableTools(ctx context.Context) []ServerTool {
132143

133144
// AvailableToolsWithoutFeatureFiltering returns tools that pass every filter
134145
// except FeatureFlagEnable/FeatureFlagDisable.
146+
//
147+
// HTTP mode uses this to build the static (process-wide) inventory, which acts
148+
// as an upper bound on what any request may see. Per-request feature flags
149+
// (from headers like X-MCP-Features or X-MCP-Insiders) are evaluated later
150+
// when the per-request inventory is derived, so feature-flagged variants must
151+
// be preserved here.
135152
func (r *Inventory) AvailableToolsWithoutFeatureFiltering(ctx context.Context) []ServerTool {
136153
var result []ServerTool
137154
for i := range r.tools {
@@ -147,12 +164,10 @@ func (r *Inventory) AvailableToolsWithoutFeatureFiltering(ctx context.Context) [
147164
}
148165

149166
func sortResourceTemplates(resourceTemplates []ServerResourceTemplate) {
150-
sort.Slice(resourceTemplates, func(i, j int) bool {
151-
if resourceTemplates[i].Toolset.ID != resourceTemplates[j].Toolset.ID {
152-
return resourceTemplates[i].Toolset.ID < resourceTemplates[j].Toolset.ID
153-
}
154-
return resourceTemplates[i].Template.Name < resourceTemplates[j].Template.Name
155-
})
167+
sortByToolsetThenName(resourceTemplates,
168+
func(r ServerResourceTemplate) ToolsetID { return r.Toolset.ID },
169+
func(r ServerResourceTemplate) string { return r.Template.Name },
170+
)
156171
}
157172

158173
// AvailableResourceTemplates returns resource templates that pass all current filters,
@@ -178,7 +193,9 @@ func (r *Inventory) AvailableResourceTemplates(ctx context.Context) []ServerReso
178193
}
179194

180195
// AvailableResourceTemplatesWithoutFeatureFiltering returns resource templates
181-
// that pass every filter except FeatureFlagEnable/FeatureFlagDisable.
196+
// that pass every filter except FeatureFlagEnable/FeatureFlagDisable. See
197+
// AvailableToolsWithoutFeatureFiltering for why feature flags are deferred in
198+
// HTTP mode.
182199
func (r *Inventory) AvailableResourceTemplatesWithoutFeatureFiltering(_ context.Context) []ServerResourceTemplate {
183200
var result []ServerResourceTemplate
184201
for i := range r.resourceTemplates {
@@ -194,12 +211,10 @@ func (r *Inventory) AvailableResourceTemplatesWithoutFeatureFiltering(_ context.
194211
}
195212

196213
func sortPrompts(prompts []ServerPrompt) {
197-
sort.Slice(prompts, func(i, j int) bool {
198-
if prompts[i].Toolset.ID != prompts[j].Toolset.ID {
199-
return prompts[i].Toolset.ID < prompts[j].Toolset.ID
200-
}
201-
return prompts[i].Prompt.Name < prompts[j].Prompt.Name
202-
})
214+
sortByToolsetThenName(prompts,
215+
func(p ServerPrompt) ToolsetID { return p.Toolset.ID },
216+
func(p ServerPrompt) string { return p.Prompt.Name },
217+
)
203218
}
204219

205220
// AvailablePrompts returns prompts that pass all current filters,
@@ -224,8 +239,10 @@ func (r *Inventory) AvailablePrompts(ctx context.Context) []ServerPrompt {
224239
return result
225240
}
226241

227-
// AvailablePromptsWithoutFeatureFiltering returns prompts that pass every filter
228-
// except FeatureFlagEnable/FeatureFlagDisable.
242+
// AvailablePromptsWithoutFeatureFiltering returns prompts that pass every
243+
// filter except FeatureFlagEnable/FeatureFlagDisable. See
244+
// AvailableToolsWithoutFeatureFiltering for why feature flags are deferred in
245+
// HTTP mode.
229246
func (r *Inventory) AvailablePromptsWithoutFeatureFiltering(_ context.Context) []ServerPrompt {
230247
var result []ServerPrompt
231248
for i := range r.prompts {

0 commit comments

Comments
 (0)