Skip to content

Commit 7c53222

Browse files
committed
refactor: treat insiders as a feature flag bundle
1 parent b070b00 commit 7c53222

17 files changed

Lines changed: 156 additions & 152 deletions

internal/ghmcp/server.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,6 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se
130130
cfg.Translator,
131131
github.FeatureFlags{
132132
LockdownMode: cfg.LockdownMode,
133-
InsidersMode: cfg.InsidersMode,
134133
},
135134
cfg.ContentWindowSize,
136135
featureChecker,
@@ -220,7 +219,7 @@ type StdioServerConfig struct {
220219
// LockdownMode indicates if we should enable lockdown mode
221220
LockdownMode bool
222221

223-
// InsidersMode indicates if we should enable experimental features
222+
// InsidersMode expands to the curated set of feature flags enabled for insiders.
224223
InsidersMode bool
225224

226225
// ExcludeTools is a list of tool names to disable regardless of other settings.
@@ -337,9 +336,12 @@ func RunStdioServer(cfg StdioServerConfig) error {
337336

338337
// createFeatureChecker returns a FeatureFlagChecker that resolves features
339338
// using the centralized ResolveFeatureFlags function. For the local server,
340-
// features are resolved once at startup from --features CLI flag + insiders mode.
339+
// features are resolved once at startup from --features CLI flag and meta flags.
341340
func createFeatureChecker(enabledFeatures []string, insidersMode bool) inventory.FeatureFlagChecker {
342-
featureSet := github.ResolveFeatureFlags(enabledFeatures, insidersMode)
341+
featureSet := github.ResolveFeatureFlags(
342+
enabledFeatures,
343+
github.MetaFeatureFlagsForInsiders(insidersMode)...,
344+
)
343345
return func(_ context.Context, flagName string) (bool, error) {
344346
return featureSet[flagName], nil
345347
}
@@ -364,9 +366,6 @@ func addUserAgentsMiddleware(cfg github.MCPServerConfig, restClient *gogithub.Cl
364366
message.Params.ClientInfo.Name,
365367
message.Params.ClientInfo.Version,
366368
)
367-
if cfg.InsidersMode {
368-
userAgent += " (insiders)"
369-
}
370369

371370
restClient.UserAgent = userAgent
372371

pkg/github/context_tools.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ func GetMe(t translations.TranslationHelperFunc) inventory.ServerTool {
105105
}
106106

107107
result := MarshalledTextResult(minimalUser)
108-
if deps.GetFlags(ctx).InsidersMode {
108+
if deps.IsFeatureEnabled(ctx, FeatureFlagIFCLabels) {
109109
if result.Meta == nil {
110110
result.Meta = mcp.Meta{}
111111
}

pkg/github/context_tools_test.go

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ func Test_GetMe(t *testing.T) {
139139
}
140140
}
141141

142-
func Test_GetMe_IFC_InsidersMode(t *testing.T) {
142+
func Test_GetMe_IFC_FeatureFlag(t *testing.T) {
143143
t.Parallel()
144144

145145
serverTool := GetMe(translations.NullTranslationHelper)
@@ -153,34 +153,41 @@ func Test_GetMe_IFC_InsidersMode(t *testing.T) {
153153
GetUser: mockResponse(t, http.StatusOK, mockUser),
154154
})
155155

156-
t.Run("insiders mode disabled omits ifc label from result meta", func(t *testing.T) {
157-
deps := BaseDeps{
158-
Client: github.NewClient(mockedHTTPClient),
159-
Flags: FeatureFlags{InsidersMode: false},
160-
}
156+
depsWithIFCFeature := func(enabled bool) *BaseDeps {
157+
return NewBaseDeps(
158+
github.NewClient(mockedHTTPClient), nil, nil, nil,
159+
translations.NullTranslationHelper,
160+
FeatureFlags{},
161+
0,
162+
func(_ context.Context, flagName string) (bool, error) {
163+
return flagName == FeatureFlagIFCLabels && enabled, nil
164+
},
165+
stubExporters(),
166+
)
167+
}
168+
169+
t.Run("feature disabled omits ifc label from result meta", func(t *testing.T) {
170+
deps := depsWithIFCFeature(false)
161171
handler := serverTool.Handler(deps)
162172

163173
request := createMCPRequest(map[string]any{})
164174
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
165175
require.NoError(t, err)
166176
require.False(t, result.IsError)
167177

168-
assert.Nil(t, result.Meta, "result meta should be nil when insiders mode is disabled")
178+
assert.Nil(t, result.Meta, "result meta should be nil when IFC labels are disabled")
169179
})
170180

171-
t.Run("insiders mode enabled includes ifc label in result meta", func(t *testing.T) {
172-
deps := BaseDeps{
173-
Client: github.NewClient(mockedHTTPClient),
174-
Flags: FeatureFlags{InsidersMode: true},
175-
}
181+
t.Run("feature enabled includes ifc label in result meta", func(t *testing.T) {
182+
deps := depsWithIFCFeature(true)
176183
handler := serverTool.Handler(deps)
177184

178185
request := createMCPRequest(map[string]any{})
179186
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
180187
require.NoError(t, err)
181188
require.False(t, result.IsError)
182189

183-
require.NotNil(t, result.Meta, "result meta should be set when insiders mode is enabled")
190+
require.NotNil(t, result.Meta, "result meta should be set when IFC labels are enabled")
184191
ifcLabel, ok := result.Meta["ifc"]
185192
require.True(t, ok, "result meta should contain ifc key")
186193

pkg/github/dependencies.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,6 @@ func (d *RequestDeps) GetT() translations.TranslationHelperFunc { return d.T }
403403
func (d *RequestDeps) GetFlags(ctx context.Context) FeatureFlags {
404404
return FeatureFlags{
405405
LockdownMode: d.lockdownMode && ghcontext.IsLockdownMode(ctx),
406-
InsidersMode: ghcontext.IsInsidersMode(ctx),
407406
}
408407
}
409408

pkg/github/feature_flags.go

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,19 @@
11
package github
22

3+
import "slices"
4+
35
// MCPAppsFeatureFlag is the feature flag name for MCP Apps (interactive UI forms).
46
const MCPAppsFeatureFlag = "remote_mcp_ui_apps"
57

68
// FeatureFlagCSVOutput is the feature flag name for CSV output on list tools.
79
const FeatureFlagCSVOutput = "csv_output"
810

11+
// FeatureFlagIFCLabels is the feature flag name for IFC security labels in tool results.
12+
const FeatureFlagIFCLabels = "ifc_labels"
13+
14+
// MetaFeatureFlagInsiders is the meta feature flag name for insiders mode.
15+
const MetaFeatureFlagInsiders = "insiders"
16+
917
// AllowedFeatureFlags is the allowlist of feature flags that can be enabled
1018
// by users via --features CLI flag or X-MCP-Features HTTP header.
1119
// Only flags in this list are accepted; unknown flags are silently ignored.
@@ -24,37 +32,45 @@ var AllowedFeatureFlags = []string{
2432
var InsidersFeatureFlags = []string{
2533
MCPAppsFeatureFlag,
2634
FeatureFlagCSVOutput,
35+
FeatureFlagIFCLabels,
36+
}
37+
38+
// MetaFeatureFlags maps meta feature flags to the concrete feature flags they enable.
39+
var MetaFeatureFlags = map[string][]string{
40+
MetaFeatureFlagInsiders: InsidersFeatureFlags,
2741
}
2842

2943
// FeatureFlags defines runtime feature toggles that adjust tool behavior.
3044
type FeatureFlags struct {
3145
LockdownMode bool
46+
47+
// Deprecated: insiders is resolved into concrete feature flags.
3248
InsidersMode bool
3349
}
3450

51+
// MetaFeatureFlagsForInsiders returns the meta feature flags enabled by insiders mode.
52+
func MetaFeatureFlagsForInsiders(enabled bool) []string {
53+
if !enabled {
54+
return nil
55+
}
56+
return []string{MetaFeatureFlagInsiders}
57+
}
58+
3559
// ResolveFeatureFlags computes the effective set of enabled feature flags by:
36-
// 1. Taking explicitly enabled features (from CLI flags or HTTP headers)
37-
// 2. Adding insiders-expanded features when insiders mode is active
38-
// 3. Validating all features against the AllowedFeatureFlags allowlist
60+
// 1. Taking explicitly enabled features validated against AllowedFeatureFlags
61+
// 2. Adding concrete features expanded from enabled meta feature flags
3962
//
4063
// Returns a set (map) for O(1) lookup by the feature checker.
41-
func ResolveFeatureFlags(enabledFeatures []string, insidersMode bool) map[string]bool {
42-
allowed := make(map[string]bool, len(AllowedFeatureFlags))
43-
for _, f := range AllowedFeatureFlags {
44-
allowed[f] = true
45-
}
46-
64+
func ResolveFeatureFlags(enabledFeatures []string, enabledMetaFeatures ...string) map[string]bool {
4765
effective := make(map[string]bool)
4866
for _, f := range enabledFeatures {
49-
if allowed[f] {
67+
if slices.Contains(AllowedFeatureFlags, f) {
5068
effective[f] = true
5169
}
5270
}
53-
if insidersMode {
54-
for _, f := range InsidersFeatureFlags {
55-
if allowed[f] {
56-
effective[f] = true
57-
}
71+
for _, metaFeature := range enabledMetaFeatures {
72+
for _, f := range MetaFeatureFlags[metaFeature] {
73+
effective[f] = true
5874
}
5975
}
6076
return effective

pkg/github/feature_flags_test.go

Lines changed: 31 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,14 @@ import (
1818
// RemoteMCPEnthusiasticGreeting is a dummy test feature flag .
1919
const RemoteMCPEnthusiasticGreeting = "remote_mcp_enthusiastic_greeting"
2020

21-
// FeatureChecker is an interface for checking if a feature flag is enabled.
22-
type FeatureChecker interface {
23-
// IsFeatureEnabled checks if a feature flag is enabled.
24-
IsFeatureEnabled(ctx context.Context, flagName string) bool
21+
func featureCheckerFor(enabledFlags ...string) func(context.Context, string) (bool, error) {
22+
enabled := make(map[string]bool, len(enabledFlags))
23+
for _, flag := range enabledFlags {
24+
enabled[flag] = true
25+
}
26+
return func(_ context.Context, flagName string) (bool, error) {
27+
return enabled[flagName], nil
28+
}
2529
}
2630

2731
// HelloWorld returns a simple greeting tool that demonstrates feature flag conditional behavior.
@@ -45,9 +49,6 @@ func HelloWorldTool(t translations.TranslationHelperFunc) inventory.ServerTool {
4549
if deps.IsFeatureEnabled(ctx, RemoteMCPEnthusiasticGreeting) {
4650
greeting += " Welcome to the future of MCP! 🎉"
4751
}
48-
if deps.GetFlags(ctx).InsidersMode {
49-
greeting += " Experimental features are enabled! 🚀"
50-
}
5152

5253
// Build response
5354
response := map[string]any{
@@ -89,12 +90,9 @@ func TestHelloWorld_ConditionalBehavior_Featureflag(t *testing.T) {
8990
t.Run(tt.name, func(t *testing.T) {
9091
t.Parallel()
9192

92-
// Create feature checker based on test case
93-
checker := func(_ context.Context, flagName string) (bool, error) {
94-
if flagName == RemoteMCPEnthusiasticGreeting {
95-
return tt.featureFlagEnabled, nil
96-
}
97-
return false, nil
93+
var enabledFlags []string
94+
if tt.featureFlagEnabled {
95+
enabledFlags = append(enabledFlags, RemoteMCPEnthusiasticGreeting)
9896
}
9997

10098
// Create deps with the checker
@@ -103,7 +101,7 @@ func TestHelloWorld_ConditionalBehavior_Featureflag(t *testing.T) {
103101
translations.NullTranslationHelper,
104102
FeatureFlags{},
105103
0,
106-
checker,
104+
featureCheckerFor(enabledFlags...),
107105
stubExporters(),
108106
)
109107

@@ -142,54 +140,62 @@ func TestResolveFeatureFlags(t *testing.T) {
142140
tests := []struct {
143141
name string
144142
enabledFeatures []string
145-
insidersMode bool
143+
metaFeatures []string
146144
expectedFlags []string
147145
unexpectedFlags []string
148146
}{
149147
{
150148
name: "no features, no insiders",
151149
enabledFeatures: nil,
152-
insidersMode: false,
153150
expectedFlags: nil,
154-
unexpectedFlags: []string{MCPAppsFeatureFlag},
151+
unexpectedFlags: []string{MCPAppsFeatureFlag, FeatureFlagIFCLabels},
155152
},
156153
{
157154
name: "explicit feature enabled",
158155
enabledFeatures: []string{MCPAppsFeatureFlag},
159-
insidersMode: false,
160156
expectedFlags: []string{MCPAppsFeatureFlag},
161157
},
162158
{
163-
name: "insiders mode enables insiders flags",
159+
name: "insiders meta feature enables insiders flags",
164160
enabledFeatures: nil,
165-
insidersMode: true,
161+
metaFeatures: []string{MetaFeatureFlagInsiders},
166162
expectedFlags: InsidersFeatureFlags,
167163
},
164+
{
165+
name: "insiders meta feature enables internal-only flags",
166+
enabledFeatures: nil,
167+
metaFeatures: []string{MetaFeatureFlagInsiders},
168+
expectedFlags: []string{FeatureFlagIFCLabels},
169+
},
170+
{
171+
name: "internal-only flags are not directly enabled",
172+
enabledFeatures: []string{FeatureFlagIFCLabels},
173+
expectedFlags: nil,
174+
unexpectedFlags: []string{FeatureFlagIFCLabels},
175+
},
168176
{
169177
name: "unknown flags are filtered out",
170178
enabledFeatures: []string{"unknown_flag", "another_unknown"},
171-
insidersMode: false,
172179
unexpectedFlags: []string{"unknown_flag", "another_unknown"},
173180
},
174181
{
175182
name: "mix of known and unknown flags",
176183
enabledFeatures: []string{MCPAppsFeatureFlag, "unknown_flag"},
177-
insidersMode: false,
178184
expectedFlags: []string{MCPAppsFeatureFlag},
179185
unexpectedFlags: []string{"unknown_flag"},
180186
},
181187
{
182188
name: "explicit plus insiders deduplicates",
183189
enabledFeatures: []string{MCPAppsFeatureFlag},
184-
insidersMode: true,
185-
expectedFlags: []string{MCPAppsFeatureFlag},
190+
metaFeatures: []string{MetaFeatureFlagInsiders},
191+
expectedFlags: InsidersFeatureFlags,
186192
},
187193
}
188194

189195
for _, tt := range tests {
190196
t.Run(tt.name, func(t *testing.T) {
191197
t.Parallel()
192-
result := ResolveFeatureFlags(tt.enabledFeatures, tt.insidersMode)
198+
result := ResolveFeatureFlags(tt.enabledFeatures, tt.metaFeatures...)
193199
for _, flag := range tt.expectedFlags {
194200
assert.True(t, result[flag], "expected flag %q to be enabled", flag)
195201
}
@@ -199,66 +205,3 @@ func TestResolveFeatureFlags(t *testing.T) {
199205
})
200206
}
201207
}
202-
203-
func TestHelloWorld_ConditionalBehavior_Config(t *testing.T) {
204-
t.Parallel()
205-
206-
tests := []struct {
207-
name string
208-
insidersMode bool
209-
expectedGreeting string
210-
}{
211-
{
212-
name: "Experimental disabled - default greeting",
213-
insidersMode: false,
214-
expectedGreeting: "Hello, world!",
215-
},
216-
{
217-
name: "Experimental enabled - experimental greeting",
218-
insidersMode: true,
219-
expectedGreeting: "Hello, world! Experimental features are enabled! 🚀",
220-
},
221-
}
222-
223-
for _, tt := range tests {
224-
t.Run(tt.name, func(t *testing.T) {
225-
t.Parallel()
226-
227-
// Create deps with the checker
228-
deps := NewBaseDeps(
229-
nil, nil, nil, nil,
230-
translations.NullTranslationHelper,
231-
FeatureFlags{InsidersMode: tt.insidersMode},
232-
0,
233-
nil,
234-
stubExporters(),
235-
)
236-
237-
// Get the tool and its handler
238-
tool := HelloWorldTool(translations.NullTranslationHelper)
239-
handler := tool.Handler(deps)
240-
241-
// Call the handler with deps in context
242-
ctx := ContextWithDeps(context.Background(), deps)
243-
result, err := handler(ctx, &mcp.CallToolRequest{
244-
Params: &mcp.CallToolParamsRaw{
245-
Arguments: json.RawMessage(`{}`),
246-
},
247-
})
248-
require.NoError(t, err)
249-
require.NotNil(t, result)
250-
require.Len(t, result.Content, 1)
251-
252-
// Parse the response - should be TextContent
253-
textContent, ok := result.Content[0].(*mcp.TextContent)
254-
require.True(t, ok, "expected content to be TextContent")
255-
256-
var response map[string]any
257-
err = json.Unmarshal([]byte(textContent.Text), &response)
258-
require.NoError(t, err)
259-
260-
// Verify the greeting matches expected based on feature flag
261-
assert.Equal(t, tt.expectedGreeting, response["greeting"])
262-
})
263-
}
264-
}

0 commit comments

Comments
 (0)