Skip to content

Commit 3af1129

Browse files
authored
adding feature flag on dependency level (#1801)
* wip injecting ff function into tool as dep * remove debug * fix linter * add better test * adding compile time check * move experimental to seperate config/ff value * adding test var * fixing test * adding flag and possibility to call feature checker * fixing name
1 parent ccf5191 commit 3af1129

File tree

9 files changed

+366
-8
lines changed

9 files changed

+366
-8
lines changed

cmd/github-mcp-server/main.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ var (
8383
LogFilePath: viper.GetString("log-file"),
8484
ContentWindowSize: viper.GetInt("content-window-size"),
8585
LockdownMode: viper.GetBool("lockdown-mode"),
86+
InsiderMode: viper.GetBool("insider-mode"),
8687
RepoAccessCacheTTL: &ttl,
8788
}
8889
return ghmcp.RunStdioServer(stdioServerConfig)
@@ -108,6 +109,7 @@ func init() {
108109
rootCmd.PersistentFlags().String("gh-host", "", "Specify the GitHub hostname (for GitHub Enterprise etc.)")
109110
rootCmd.PersistentFlags().Int("content-window-size", 5000, "Specify the content window size")
110111
rootCmd.PersistentFlags().Bool("lockdown-mode", false, "Enable lockdown mode")
112+
rootCmd.PersistentFlags().Bool("insider-mode", false, "Enable insider features")
111113
rootCmd.PersistentFlags().Duration("repo-access-cache-ttl", 5*time.Minute, "Override the repo access cache TTL (e.g. 1m, 0s to disable)")
112114

113115
// Bind flag to viper
@@ -122,6 +124,7 @@ func init() {
122124
_ = viper.BindPFlag("host", rootCmd.PersistentFlags().Lookup("gh-host"))
123125
_ = viper.BindPFlag("content-window-size", rootCmd.PersistentFlags().Lookup("content-window-size"))
124126
_ = viper.BindPFlag("lockdown-mode", rootCmd.PersistentFlags().Lookup("lockdown-mode"))
127+
_ = viper.BindPFlag("insider-mode", rootCmd.PersistentFlags().Lookup("insider-mode"))
125128
_ = viper.BindPFlag("repo-access-cache-ttl", rootCmd.PersistentFlags().Lookup("repo-access-cache-ttl"))
126129

127130
// Add subcommands

internal/ghmcp/server.go

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ type MCPServerConfig struct {
6464
// LockdownMode indicates if we should enable lockdown mode
6565
LockdownMode bool
6666

67+
// Insider indicates if we should enable experimental features
68+
InsiderMode bool
69+
6770
// Logger is used for logging within the server
6871
Logger *slog.Logger
6972
// RepoAccessTTL overrides the default TTL for repository access cache entries.
@@ -198,15 +201,22 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
198201
ghServer.AddReceivingMiddleware(addGitHubAPIErrorToContext)
199202
ghServer.AddReceivingMiddleware(addUserAgentsMiddleware(cfg, clients.rest, clients.gqlHTTP))
200203

204+
// Create feature checker
205+
featureChecker := createFeatureChecker(cfg.EnabledFeatures)
206+
201207
// Create dependencies for tool handlers
202208
deps := github.NewBaseDeps(
203209
clients.rest,
204210
clients.gql,
205211
clients.raw,
206212
clients.repoAccess,
207213
cfg.Translator,
208-
github.FeatureFlags{LockdownMode: cfg.LockdownMode},
214+
github.FeatureFlags{
215+
LockdownMode: cfg.LockdownMode,
216+
InsiderMode: cfg.InsiderMode,
217+
},
209218
cfg.ContentWindowSize,
219+
featureChecker,
210220
)
211221

212222
// Inject dependencies into context for all tool handlers
@@ -222,8 +232,8 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
222232
WithReadOnly(cfg.ReadOnly).
223233
WithToolsets(enabledToolsets).
224234
WithTools(cfg.EnabledTools).
225-
WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures))
226-
235+
WithFeatureChecker(featureChecker)
236+
227237
// Apply token scope filtering if scopes are known (for PAT filtering)
228238
if cfg.TokenScopes != nil {
229239
inventoryBuilder = inventoryBuilder.WithFilter(github.CreateToolScopeFilter(cfg.TokenScopes))
@@ -325,6 +335,9 @@ type StdioServerConfig struct {
325335
// LockdownMode indicates if we should enable lockdown mode
326336
LockdownMode bool
327337

338+
// InsiderMode indicates if we should enable experimental features
339+
InsiderMode bool
340+
328341
// RepoAccessCacheTTL overrides the default TTL for repository access cache entries.
329342
RepoAccessCacheTTL *time.Duration
330343
}
@@ -381,6 +394,7 @@ func RunStdioServer(cfg StdioServerConfig) error {
381394
Translator: t,
382395
ContentWindowSize: cfg.ContentWindowSize,
383396
LockdownMode: cfg.LockdownMode,
397+
InsiderMode: cfg.InsiderMode,
384398
Logger: logger,
385399
RepoAccessTTL: cfg.RepoAccessCacheTTL,
386400
TokenScopes: tokenScopes,

internal/ghmcp/server_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) {
2323
Translator: translations.NullTranslationHelper,
2424
ContentWindowSize: 5000,
2525
LockdownMode: false,
26+
InsiderMode: false,
2627
}
2728

2829
// Create the server

pkg/github/dependencies.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package github
33
import (
44
"context"
55
"errors"
6+
"fmt"
7+
"os"
68

79
"github.com/github/github-mcp-server/pkg/inventory"
810
"github.com/github/github-mcp-server/pkg/lockdown"
@@ -77,6 +79,9 @@ type ToolDependencies interface {
7779

7880
// GetContentWindowSize returns the content window size for log truncation
7981
GetContentWindowSize() int
82+
83+
// IsFeatureEnabled checks if a feature flag is enabled.
84+
IsFeatureEnabled(ctx context.Context, flagName string) bool
8085
}
8186

8287
// BaseDeps is the standard implementation of ToolDependencies for the local server.
@@ -93,8 +98,14 @@ type BaseDeps struct {
9398
T translations.TranslationHelperFunc
9499
Flags FeatureFlags
95100
ContentWindowSize int
101+
102+
// Feature flag checker for runtime checks
103+
featureChecker inventory.FeatureFlagChecker
96104
}
97105

106+
// Compile-time assertion to verify that BaseDeps implements the ToolDependencies interface.
107+
var _ ToolDependencies = (*BaseDeps)(nil)
108+
98109
// NewBaseDeps creates a BaseDeps with the provided clients and configuration.
99110
func NewBaseDeps(
100111
client *gogithub.Client,
@@ -104,6 +115,7 @@ func NewBaseDeps(
104115
t translations.TranslationHelperFunc,
105116
flags FeatureFlags,
106117
contentWindowSize int,
118+
featureChecker inventory.FeatureFlagChecker,
107119
) *BaseDeps {
108120
return &BaseDeps{
109121
Client: client,
@@ -113,6 +125,7 @@ func NewBaseDeps(
113125
T: t,
114126
Flags: flags,
115127
ContentWindowSize: contentWindowSize,
128+
featureChecker: featureChecker,
116129
}
117130
}
118131

@@ -143,6 +156,24 @@ func (d BaseDeps) GetFlags() FeatureFlags { return d.Flags }
143156
// GetContentWindowSize implements ToolDependencies.
144157
func (d BaseDeps) GetContentWindowSize() int { return d.ContentWindowSize }
145158

159+
// IsFeatureEnabled checks if a feature flag is enabled.
160+
// Returns false if the feature checker is nil, flag name is empty, or an error occurs.
161+
// This allows tools to conditionally change behavior based on feature flags.
162+
func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flagName string) bool {
163+
if d.featureChecker == nil || flagName == "" {
164+
return false
165+
}
166+
167+
enabled, err := d.featureChecker(ctx, flagName)
168+
if err != nil {
169+
// Log error but don't fail the tool - treat as disabled
170+
fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", flagName, err)
171+
return false
172+
}
173+
174+
return enabled
175+
}
176+
146177
// NewTool creates a ServerTool that retrieves ToolDependencies from context at call time.
147178
// This avoids creating closures at registration time, which is important for performance
148179
// in servers that create a new server instance per request (like the remote server).

pkg/github/dependencies_test.go

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
package github_test
2+
3+
import (
4+
"context"
5+
"errors"
6+
"testing"
7+
8+
"github.com/github/github-mcp-server/pkg/github"
9+
"github.com/github/github-mcp-server/pkg/translations"
10+
"github.com/stretchr/testify/assert"
11+
)
12+
13+
func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) {
14+
t.Parallel()
15+
16+
// Create a feature checker that returns true for "test_flag"
17+
checker := func(_ context.Context, flagName string) (bool, error) {
18+
return flagName == "test_flag", nil
19+
}
20+
21+
// Create deps with the checker using NewBaseDeps
22+
deps := github.NewBaseDeps(
23+
nil, // client
24+
nil, // gqlClient
25+
nil, // rawClient
26+
nil, // repoAccessCache
27+
translations.NullTranslationHelper,
28+
github.FeatureFlags{},
29+
0, // contentWindowSize
30+
checker, // featureChecker
31+
)
32+
33+
// Test enabled flag
34+
result := deps.IsFeatureEnabled(context.Background(), "test_flag")
35+
assert.True(t, result, "Expected test_flag to be enabled")
36+
37+
// Test disabled flag
38+
result = deps.IsFeatureEnabled(context.Background(), "other_flag")
39+
assert.False(t, result, "Expected other_flag to be disabled")
40+
}
41+
42+
func TestIsFeatureEnabled_WithoutChecker(t *testing.T) {
43+
t.Parallel()
44+
45+
// Create deps without feature checker (nil)
46+
deps := github.NewBaseDeps(
47+
nil, // client
48+
nil, // gqlClient
49+
nil, // rawClient
50+
nil, // repoAccessCache
51+
translations.NullTranslationHelper,
52+
github.FeatureFlags{},
53+
0, // contentWindowSize
54+
nil, // featureChecker (nil)
55+
)
56+
57+
// Should return false when checker is nil
58+
result := deps.IsFeatureEnabled(context.Background(), "any_flag")
59+
assert.False(t, result, "Expected false when checker is nil")
60+
}
61+
62+
func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) {
63+
t.Parallel()
64+
65+
// Create a feature checker
66+
checker := func(_ context.Context, _ string) (bool, error) {
67+
return true, nil
68+
}
69+
70+
deps := github.NewBaseDeps(
71+
nil, // client
72+
nil, // gqlClient
73+
nil, // rawClient
74+
nil, // repoAccessCache
75+
translations.NullTranslationHelper,
76+
github.FeatureFlags{},
77+
0, // contentWindowSize
78+
checker, // featureChecker
79+
)
80+
81+
// Should return false for empty flag name
82+
result := deps.IsFeatureEnabled(context.Background(), "")
83+
assert.False(t, result, "Expected false for empty flag name")
84+
}
85+
86+
func TestIsFeatureEnabled_CheckerError(t *testing.T) {
87+
t.Parallel()
88+
89+
// Create a feature checker that returns an error
90+
checker := func(_ context.Context, _ string) (bool, error) {
91+
return false, errors.New("checker error")
92+
}
93+
94+
deps := github.NewBaseDeps(
95+
nil, // client
96+
nil, // gqlClient
97+
nil, // rawClient
98+
nil, // repoAccessCache
99+
translations.NullTranslationHelper,
100+
github.FeatureFlags{},
101+
0, // contentWindowSize
102+
checker, // featureChecker
103+
)
104+
105+
// Should return false and log error (not crash)
106+
result := deps.IsFeatureEnabled(context.Background(), "error_flag")
107+
assert.False(t, result, "Expected false when checker returns error")
108+
}

pkg/github/dynamic_tools_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ func TestDynamicTools_EnableToolset(t *testing.T) {
136136
deps := DynamicToolDependencies{
137137
Server: server,
138138
Inventory: reg,
139-
ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0),
139+
ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0, nil),
140140
T: translations.NullTranslationHelper,
141141
}
142142

pkg/github/feature_flags.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,5 @@ package github
33
// FeatureFlags defines runtime feature toggles that adjust tool behavior.
44
type FeatureFlags struct {
55
LockdownMode bool
6+
InsiderMode bool
67
}

0 commit comments

Comments
 (0)